From 739c9f298a2a4e6500703c431f4b6a8c56b64094 Mon Sep 17 00:00:00 2001 From: Thomas Nagler Date: Sat, 12 Sep 2026 11:20:46 +0200 Subject: [PATCH 1/3] test: cover the drop-everything path of `remove_nans` `remove_nans` shrinks `x` and `weights` with `conservativeResize`, which for a dynamic vector reaches Eigen's `aligned_realloc` and calls `std::realloc(ptr, new_size)` directly. When every row is dropped -- every observation NaN, every weight a drop marker, or an already empty `x` -- `new_size` is zero. `realloc(ptr, 0)` is deprecated in C17 and undefined in C23, and valgrind reports it as an error. CRAN's valgrind check of the R package hit this through a zero-inflated fit whose observations were all zero: `fit` zeroes the weight of every zero observation, marks those rows NaN, and calls `remove_nans` a second time, which empties both vectors. https://www.stats.ox.ac.uk/pub/bdr/memtests/valgrind/kde1d/tests/testthat.Rout Nothing in this suite noticed, because a zero-size `realloc` returns normally -- the tests only fail under a memory checker. So this adds both the cases and a CI job that runs the suite under `valgrind --error-exitcode=1`. Both currently fail there. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/main.yml | 39 ++++++++++++++++++++++++++ test/test.cpp | 57 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 1fe19f8..2cd8e5a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -115,3 +115,42 @@ 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. CRAN runs + # the R package's tests under valgrind and reports such calls as errors, so + # this suite has to be run the same way to catch them here first. + valgrind: + runs-on: ubuntu-latest + name: ubuntu-latest (valgrind) + env: + BOOST_VERSION: 1.84.0 + EIGEN_VERSION: 3.4.0 + 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 + run: sudo apt-get update && sudo apt-get install -y valgrind + - name: Build and run the tests under valgrind + run: | + cmake -S . -B valgrind-build -DCMAKE_BUILD_TYPE=Release -DOPT_ASAN=OFF + cmake --build valgrind-build + for t in kde1d-unit-test kde1d-invariant-test; do + valgrind --error-exitcode=1 --errors-for-leak-kinds=none \ + valgrind-build/bin/$t + done + env: + CMAKE_PREFIX_PATH: /home/runner/work 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); From 2ec43decc3ab0e210c83b29860acad7425d53506 Mon Sep 17 00:00:00 2001 From: Thomas Nagler Date: Sat, 12 Sep 2026 11:22:58 +0200 Subject: [PATCH 2/3] fix: do not resize to zero through `conservativeResize` When every row is a drop marker, `last` wraps to `SIZE_MAX` and the new size is zero. Eigen's `conservativeResize` reaches `aligned_realloc` for a dynamic vector, so that became `std::realloc(ptr, 0)` -- deprecated in C17, undefined in C23, and an error under valgrind. Swap in a fresh empty vector instead. That only exchanges storage pointers, and the old buffer is released by `free` when the temporary dies. The resulting sizes are unchanged, so every caller sees what it saw before. Co-Authored-By: Claude Opus 5 (1M context) --- include/kde1d/tools.hpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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); From ce735349e15f6bba05ede94fad38f18736be7398 Mon Sep 17 00:00:00 2001 From: Thomas Nagler Date: Sat, 12 Sep 2026 12:21:24 +0200 Subject: [PATCH 3/3] ci: run the test suite under the sanitizers as well as valgrind Valgrind catches the zero-size `realloc` this branch fixes, but that is one narrow class of defect. Signed overflow, bad shifts, invalid casts, stack errors and leaks are all things it either cannot see or does not report, and the suite has never been run under a sanitizer in CI -- every existing job passes `-DOPT_ASAN=OFF`. `OPT_ASAN` could not have been turned on instead: it only appends to `CMAKE_CXX_FLAGS_DEBUG`, so it is silently inert in the release build the tests actually run, and it cannot request anything but the address sanitizer. So this adds a `SANITIZERS` cache variable taking an explicit `-fsanitize=` list, applied to every configuration, with `-fno-sanitize-recover=all` so a finding exits nonzero -- UBSan otherwise prints a diagnostic and carries on, and CI would pass. The valgrind job becomes a `memory-checks` matrix: valgrind, and address+undefined under both GCC and Clang (the leak sanitizer comes with the address sanitizer on Linux). Valgrind and the address sanitizer both instrument allocation and cannot share a process, hence separate entries; GCC and Clang disagree often enough about what UBSan instruments to be worth both. All three entries pass locally on this branch: 587 + 721 assertions, no findings. Verified the sanitizer entries fail as intended by temporarily introducing a signed overflow and a heap overread into the suite. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/main.yml | 60 +++++++++++++++++++++++++++++++------- cmake/compilerDefOpt.cmake | 11 +++++++ cmake/options.cmake | 6 ++++ 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2cd8e5a..a1ac395 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -117,15 +117,34 @@ jobs: 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. CRAN runs - # the R package's tests under valgrind and reports such calls as errors, so - # this suite has to be run the same way to catch them here first. - valgrind: + # 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 (valgrind) + 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 @@ -143,14 +162,33 @@ jobs: 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 and run the tests under 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 valgrind-build -DCMAKE_BUILD_TYPE=Release -DOPT_ASAN=OFF - cmake --build valgrind-build + 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 - valgrind --error-exitcode=1 --errors-for-leak-kinds=none \ - valgrind-build/bin/$t + 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: - CMAKE_PREFIX_PATH: /home/runner/work + 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.")