From 69cb5b7bc7567f015dd59dce98a5fd93a79f4062 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 24 Jan 2018 18:26:54 -0500 Subject: [PATCH 1/4] Start C++ stream reader tests for malformed inputs [ci skip] Change-Id: I6af65bb1fe46f8aa815895570849fcef5b624bcc --- cpp/src/arrow/ipc/ipc-read-write-test.cc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/cpp/src/arrow/ipc/ipc-read-write-test.cc b/cpp/src/arrow/ipc/ipc-read-write-test.cc index 1fcbdac5ebc..c0fa89efe98 100644 --- a/cpp/src/arrow/ipc/ipc-read-write-test.cc +++ b/cpp/src/arrow/ipc/ipc-read-write-test.cc @@ -755,5 +755,18 @@ TEST_F(TestTensorRoundTrip, NonContiguous) { CheckTensorRoundTrip(tensor); } +TEST(TestRecordBatchStreamReader, MalformedInput) { + auto empty = std::make_shared(""); + auto garbage = std::make_shared("12345678"); + + std::shared_ptr batch_reader; + + io::BufferReader empty_reader(empty); + ASSERT_RAISES(Invalid, RecordBatchStreamReader::Open(&empty_reader, &batch_reader)); + + io::BufferReader garbage_reader(garbage); + ASSERT_RAISES(Invalid, RecordBatchStreamReader::Open(&garbage_reader, &batch_reader)); +} + } // namespace ipc } // namespace arrow From 53d2c9210712d672e9374e2197bbaa6a93afe899 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Feb 2018 14:37:31 -0500 Subject: [PATCH 2/4] Add missing lsan-suppressions.txt to enable ASAN to run with unittests [skip ci] Change-Id: I007e567af807d2af4d1801a11cd34279d80fd740 --- cpp/CMakeLists.txt | 7 ++++--- cpp/build-support/lsan-suppressions.txt | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) create mode 100644 cpp/build-support/lsan-suppressions.txt diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index b9221414bf6..8c0e95634fc 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -291,8 +291,6 @@ include(ThirdpartyToolchain) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX_COMMON_FLAGS}") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ARROW_CXXFLAGS}") -message(STATUS "CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}") - if ("${COMPILER_FAMILY}" STREQUAL "clang") # Using Clang with ccache causes a bunch of spurious warnings that are # purportedly fixed in the next version of ccache. See the following for details: @@ -305,7 +303,7 @@ endif() # ASAN / TSAN / UBSAN if(ARROW_FUZZING) - set(ARROW_USE_COVERAGE ON) + set(ARROW_USE_COVERAGE ON) endif() include(san-config) @@ -333,6 +331,9 @@ if ("${ARROW_GENERATE_COVERAGE}") endif() endif() +# CMAKE_CXX_FLAGS now fully assembled +message(STATUS "CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}") + # set compile output directory string (TOLOWER ${CMAKE_BUILD_TYPE} BUILD_SUBDIR_NAME) diff --git a/cpp/build-support/lsan-suppressions.txt b/cpp/build-support/lsan-suppressions.txt new file mode 100644 index 00000000000..927afb39710 --- /dev/null +++ b/cpp/build-support/lsan-suppressions.txt @@ -0,0 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# False positive from atexit() registration in libc +leak:*__new_exitfn* From 90787ec89f0defb6a212ee2e418fef499a9b7ff5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Feb 2018 14:58:42 -0500 Subject: [PATCH 3/4] Add ASAN instructions, llvm-symbolizer instructions for fuzzers. Remove Apache Kudu cruft from run-test.sh Change-Id: Ib97dbdb32e3f4da4919584e29f17db55bfb964a5 --- cpp/README.md | 25 ++++++++++++++++++++++++- cpp/build-support/run-test.sh | 6 ------ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/cpp/README.md b/cpp/README.md index 1daf863819c..daeeade7230 100644 --- a/cpp/README.md +++ b/cpp/README.md @@ -99,7 +99,14 @@ and benchmarks or `make runbenchmark` to run only the benchmark tests. Benchmark logs will be placed in the build directory under `build/benchmark-logs`. -## Building/Running fuzzers +### Testing with LLVM AddressSanitizer + +To use AddressSanitizer (ASAN) to find bad memory accesses or leaks with LLVM, +pass `-DARROW_USE_ASAN=ON` when building. You must use clang to compile with +ASAN, and `ARROW_USE_ASAN` is mutually-exclusive with the valgrind option +`ARROW_TEST_MEMCHECK`. + +### Building/Running fuzzers Fuzzers can help finding unhandled exceptions and problems with untrusted input that may lead to crashes, security issues and undefined behavior. They do this by @@ -128,6 +135,22 @@ stack trace as well as the input data. After a problem was found this way, it sh be reported and fixed. Usually, the fuzzing process cannot be continued until the fix is applied, since the fuzzer usually converts to the problem again. +If you build fuzzers with ASAN, you need to set the `ASAN_SYMBOLIZER_PATH` +environment variable to the absolute path of `llvm-symbolizer`, which is a tool +that ships with LLVM. + +```shell +export ASAN_SYMBOLIZER_PATH=$(type -p llvm-symbolizer) +``` + +Note that some fuzzer builds currently reject paths with a version qualifier +(like `llvm-sanitizer-5.0`). To overcome this, set an appropriate symlink +(here, when using LLVM 5.0): + +```shell +ln -sf /usr/bin/llvm-sanitizer-5.0 /usr/bin/llvm-sanitizer +``` + There are some problems that may occur during the compilation process: - libfuzzer was not distributed with your LLVM: `ld: file not found: .../libLLVMFuzzer.a` diff --git a/cpp/build-support/run-test.sh b/cpp/build-support/run-test.sh index b4da4f3f02e..656ab7bd3b8 100755 --- a/cpp/build-support/run-test.sh +++ b/cpp/build-support/run-test.sh @@ -88,12 +88,6 @@ function setup_sanitizers() { # Set up suppressions for LeakSanitizer LSAN_OPTIONS="$LSAN_OPTIONS suppressions=$ROOT/build-support/lsan-suppressions.txt" export LSAN_OPTIONS - - # Suppressions require symbolization. We'll default to using the symbolizer in - # thirdparty. - if [ -z "$ASAN_SYMBOLIZER_PATH" ]; then - export ASAN_SYMBOLIZER_PATH=$(find $NATIVE_TOOLCHAIN/llvm-3.7.0/bin -name llvm-symbolizer) - fi } function run_test() { From e3a590ed244b4a306420434f64869a054402d027 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Feb 2018 15:42:39 -0500 Subject: [PATCH 4/4] Do not invoke memcpy unless number of bytes to read greater than 0. Return more informative error message Change-Id: I335736571e56af36f4de909e35f8d667693db7fe --- cpp/src/arrow/io/memory.cc | 6 ++++-- cpp/src/arrow/ipc/ipc-read-write-test.cc | 7 +++++-- cpp/src/arrow/ipc/message.cc | 5 ++++- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/io/memory.cc b/cpp/src/arrow/io/memory.cc index ecdf26f0a99..45d6bdbde3b 100644 --- a/cpp/src/arrow/io/memory.cc +++ b/cpp/src/arrow/io/memory.cc @@ -257,9 +257,11 @@ Status BufferReader::Tell(int64_t* position) const { bool BufferReader::supports_zero_copy() const { return true; } Status BufferReader::Read(int64_t nbytes, int64_t* bytes_read, void* buffer) { - memcpy(buffer, data_ + position_, nbytes); *bytes_read = std::min(nbytes, size_ - position_); - position_ += *bytes_read; + if (*bytes_read) { + memcpy(buffer, data_ + position_, *bytes_read); + position_ += *bytes_read; + } return Status::OK(); } diff --git a/cpp/src/arrow/ipc/ipc-read-write-test.cc b/cpp/src/arrow/ipc/ipc-read-write-test.cc index c0fa89efe98..d877e9922e3 100644 --- a/cpp/src/arrow/ipc/ipc-read-write-test.cc +++ b/cpp/src/arrow/ipc/ipc-read-write-test.cc @@ -756,8 +756,11 @@ TEST_F(TestTensorRoundTrip, NonContiguous) { } TEST(TestRecordBatchStreamReader, MalformedInput) { - auto empty = std::make_shared(""); - auto garbage = std::make_shared("12345678"); + const std::string empty_str = ""; + const std::string garbage_str = "12345678"; + + auto empty = std::make_shared(empty_str); + auto garbage = std::make_shared(garbage_str); std::shared_ptr batch_reader; diff --git a/cpp/src/arrow/ipc/message.cc b/cpp/src/arrow/ipc/message.cc index 1835cefde09..fd747563a45 100644 --- a/cpp/src/arrow/ipc/message.cc +++ b/cpp/src/arrow/ipc/message.cc @@ -227,7 +227,10 @@ Status ReadMessage(io::InputStream* file, std::unique_ptr* message) { std::shared_ptr metadata; RETURN_NOT_OK(file->Read(message_length, &metadata)); if (metadata->size() != message_length) { - return Status::IOError("Unexpected end of stream trying to read message"); + std::stringstream ss; + ss << "Expected to read " << message_length << " metadata bytes, but " + << "only read " << metadata->size(); + return Status::Invalid(ss.str()); } return Message::ReadFrom(metadata, file, message);