fix: avoid zero-size realloc in remove_nans

.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

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})

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.")

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);

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);