diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 1fe19f8..a1ac395 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -115,3 +115,80 @@ jobs: cmake --build consumer ./consumer/consumer shell: bash + + # The unit tests pass on inputs whose memory behaviour is still wrong: a + # zero-size `realloc` is undefined in C23, but it returns normally, and the + # same is true of most undefined behaviour. CRAN runs the R package's tests + # under valgrind and reports such calls as errors, so this suite has to be + # run under a checker to catch them here first. + # + # Valgrind and the address sanitizer both instrument allocation and cannot + # share a process, so they are separate matrix entries. They also do not see + # the same things: valgrind catches the zero-size `realloc` above, which no + # sanitizer flags, while the sanitizers catch signed overflow, bad shifts, + # invalid casts and stack errors that valgrind cannot see. The leak + # sanitizer comes with the address sanitizer on Linux, and GCC and Clang + # disagree often enough about what UBSan instruments to be worth both. + memory-checks: + strategy: + fail-fast: false + matrix: + cfg: + - { name: 'valgrind', cc: gcc, cxx: g++, sanitizers: '' } + - { name: 'asan+lsan+ubsan GNU', cc: gcc, cxx: g++, sanitizers: 'address,undefined' } + - { name: 'asan+lsan+ubsan Clang', cc: clang, cxx: clang++, sanitizers: 'address,undefined' } + runs-on: ubuntu-latest + name: ubuntu-latest (${{ matrix.cfg.name }}) + env: + BOOST_VERSION: 1.84.0 + EIGEN_VERSION: 3.4.0 + CMAKE_PREFIX_PATH: /home/runner/work + CC: ${{ matrix.cfg.cc }} + CXX: ${{ matrix.cfg.cxx }} + steps: + - name: Checkout project + uses: actions/checkout@v4 + with: + submodules: recursive + - name: Install dependencies + id: install-dependencies + uses: vinecopulib/vinecopulib/.github/actions/install-dependencies@main + with: + os: ubuntu-latest + platform: x64 + boost_install_dir: '/home/runner/work' + eigen_install_dir: '/home/runner/work' + boost_version: ${{ env.BOOST_VERSION }} + eigen_version: ${{ env.EIGEN_VERSION }} + wdm: 'false' + - name: Install valgrind + if: matrix.cfg.sanitizers == '' + run: sudo apt-get update && sudo apt-get install -y valgrind + - name: Build + # `OPT_ASAN` is on by default and would add `-fsanitize=address` to the + # debug flags, which the valgrind entry must not carry. + run: | + cmake -S . -B check -DCMAKE_BUILD_TYPE=Release -DOPT_ASAN=OFF \ + -DSANITIZERS='${{ matrix.cfg.sanitizers }}' + cmake --build check -j + shell: bash + - name: Run the tests under ${{ matrix.cfg.name }} + # Both binaries run even if the first fails, so one report does not + # hide the other; the loop keeps the worst exit status. + run: | + status=0 + for t in kde1d-unit-test kde1d-invariant-test; do + echo "::group::$t" + if [ -z '${{ matrix.cfg.sanitizers }}' ]; then + valgrind --error-exitcode=1 --errors-for-leak-kinds=none \ + check/bin/$t || status=1 + else + check/bin/$t || status=1 + fi + echo "::endgroup::" + done + exit $status + shell: bash + env: + ASAN_OPTIONS: detect_leaks=1:detect_stack_use_after_return=1 + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1 diff --git a/cmake/compilerDefOpt.cmake b/cmake/compilerDefOpt.cmake index 34a04e9..135d741 100755 --- a/cmake/compilerDefOpt.cmake +++ b/cmake/compilerDefOpt.cmake @@ -29,6 +29,17 @@ if(NOT WIN32) endif() endif() + if(SANITIZERS) + # Sanitizer flags belong to every configuration, not just Debug: the + # release build is the one whose optimizations the checks have to + # survive. `-fno-sanitize-recover` makes a finding exit nonzero -- + # UBSan otherwise prints a diagnostic and carries on, so CI would pass. + # Frame pointers and line tables are what make the reports readable. + add_compile_options(-fsanitize=${SANITIZERS} -fno-sanitize-recover=all + -fno-omit-frame-pointer -g) + add_link_options(-fsanitize=${SANITIZERS}) + endif() + if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") if(NOT EXISTS ${CMAKE_CXX_COMPILER}) diff --git a/cmake/options.cmake b/cmake/options.cmake index da71e6c..129dd84 100755 --- a/cmake/options.cmake +++ b/cmake/options.cmake @@ -10,3 +10,9 @@ option(OPT_ASAN "Use adress sanitizer (debug)" "ON") option(BUILD_TESTING "Build tests." "ON") option(BUILD_BENCHMARKS "Build benchmarks." "OFF") option(CODE_COVERAGE "Code coverage." "OFF") + +# `OPT_ASAN` only reaches `CMAKE_CXX_FLAGS_DEBUG`, so it is silently inert in a +# release build, and it cannot ask for anything but the address sanitizer. This +# takes an explicit `-fsanitize=` list and applies it to every build type. +set(SANITIZERS "" CACHE STRING + "Comma-separated -fsanitize= list, e.g. address,undefined. Empty disables.") diff --git a/include/kde1d/tools.hpp b/include/kde1d/tools.hpp index db2d700..710f450 100644 --- a/include/kde1d/tools.hpp +++ b/include/kde1d/tools.hpp @@ -82,6 +82,20 @@ remove_nans(Eigen::VectorXd& x, Eigen::VectorXd& weights) } // remove nan rows + // + // `last` is unsigned, so it wraps to `SIZE_MAX` when every row is dropped + // (or when `x` arrived empty) and the new size is zero. `conservativeResize` + // would then reach Eigen's `aligned_realloc`, which calls + // `std::realloc(ptr, 0)` -- deprecated in C17, undefined in C23, and + // reported as an error by valgrind. Swapping in a fresh empty vector only + // exchanges storage pointers; the old buffer is released by `free`. + if (last + 1 == 0) { + Eigen::VectorXd().swap(x); + if (weights.size() > 0) + Eigen::VectorXd().swap(weights); + return; + } + x.conservativeResize(last + 1); if (weights.size() > 0) weights.conservativeResize(last + 1); diff --git a/test/test.cpp b/test/test.cpp index 0de5b95..67553b2 100644 --- a/test/test.cpp +++ b/test/test.cpp @@ -280,6 +280,63 @@ TEST_CASE("two-sided exploding endpoints retain the bulk fit", CHECK(fit.get_edf() == Approx(bulk.get_edf()).epsilon(1e-12)); } +TEST_CASE("dropping every row does not reallocate to size zero", "[tools]") +{ + // `x.conservativeResize(n)` with `n == 0` reaches Eigen's `aligned_realloc`, + // which calls `std::realloc(ptr, 0)`. That is deprecated in C17 and + // undefined in C23, and valgrind reports it as an error -- which is how CRAN + // found it in the R package, via a zero-inflated fit whose observations were + // all zero. Run this suite under valgrind to see the regression; the size + // checks below only pin down the behaviour the fix has to preserve. + const Eigen::VectorXd all_nan = Eigen::VectorXd::Constant(20, NAN); + const Eigen::VectorXd ones = Eigen::VectorXd::Ones(20); + + SECTION("every observation is NaN") + { + Eigen::VectorXd x = all_nan; + Eigen::VectorXd w = ones; + tools::remove_nans(x, w); + CHECK(x.size() == 0); + CHECK(w.size() == 0); + } + + SECTION("every weight is a drop marker") + { + Eigen::VectorXd x = Eigen::VectorXd::LinSpaced(20, -2.0, 2.0); + Eigen::VectorXd w = Eigen::VectorXd::Zero(20); + tools::remove_nans(x, w); + CHECK(x.size() == 0); + CHECK(w.size() == 0); + } + + SECTION("no weights are given") + { + Eigen::VectorXd x = all_nan; + Eigen::VectorXd w; + tools::remove_nans(x, w); + CHECK(x.size() == 0); + CHECK(w.size() == 0); + } + + SECTION("x is already empty") + { + Eigen::VectorXd x; + Eigen::VectorXd w; + tools::remove_nans(x, w); + CHECK(x.size() == 0); + } + + SECTION("a zero-inflated fit sees only zeros") + { + // The path CRAN reported: `fit` zeroes the weight of every zero + // observation, marks those rows NaN and calls `remove_nans` a second time, + // which here empties both vectors. + Kde1d fit(0.0, NAN, "zero_inflated"); + CHECK_NOTHROW(fit.fit(Eigen::VectorXd::Zero(20))); + CHECK(std::isnan(fit.get_bandwidth())); + } +} + TEST_CASE("inputs that leave nothing to fit are refused", "[kde1d]") { const Eigen::VectorXd x = Eigen::VectorXd::LinSpaced(20, -2.0, 2.0);