diff --git a/.github/workflows/sanitizers.yml b/.github/workflows/sanitizers.yml index aef0c39..7224845 100644 --- a/.github/workflows/sanitizers.yml +++ b/.github/workflows/sanitizers.yml @@ -9,11 +9,12 @@ name: ๐Ÿงผ Sanitizers (Linux) # overflow, no double free, on either path. There is no backlog to clear # first, and the point of turning it on now is to keep it that way. See #162. # -# Leaks are a separate matter and are deliberately NOT blocking: nothing owns -# a PropagationModel (#159), so ASan reports every one as leaked. Making that -# fail the build would produce a permanently red job, which teaches everyone -# to ignore it. The leak check runs anyway, as an informational step, so the -# number stays visible and can be made blocking once #159 lands. +# Leaks are blocking for the unit suite. They were not when this job landed โ€” +# nothing owned a PropagationModel, so ASan reported every one as leaked, about +# 4.7 MB in all. #159 gave the models, the propagative layer and two orphaned +# constant-layer arrays an owner, which took the unit suite to zero, so the +# gate went on. runff still leaks ~200 kB on paths the unit suite does not +# reach, so its leak check stays informational until those are found too. # # A separate workflow rather than a job in main.yml because it needs its own # build: install-forefire.sh builds with -O3 -flto -fomit-frame-pointer, which @@ -55,13 +56,16 @@ jobs: cmake -S . -B build-asan -DFOREFIRE_SANITIZE=address cmake --build build-asan -j"$(nproc)" - # Errors are blocking. detect_leaks=0 is the whole reason this can be: - # see the note at the top about #159. + # Leaks are blocking here: the unit suite reports none since the model + # and layer ownership fix in #159, and this is what keeps it that way. - name: Unit tests under ASan env: - ASAN_OPTIONS: detect_leaks=0 + ASAN_OPTIONS: detect_leaks=1 run: ctest --test-dir build-asan --output-on-failure + # runff still leaks about 200 kB over 783 allocations, on paths a single + # simulation reaches and the unit suite does not, so its leak check stays + # informational below. Errors are blocking here as they always were. - name: runff under ASan env: ASAN_OPTIONS: detect_leaks=0 @@ -69,13 +73,14 @@ jobs: cd tests/runff bash ff-run.bash - # Informational only. Prints what is leaking so the figure stays visible - # without gating the build on a known ownership gap. - - name: Leak report (informational) + # Informational only, and only for runff now. Keeps the figure visible so + # the remaining ownership gaps can be found and this can join the gate. + - name: runff leak report (informational) continue-on-error: true env: ASAN_OPTIONS: detect_leaks=1 run: | - echo "::group::Unit suite leaks" - ctest --test-dir build-asan --output-on-failure || true + echo "::group::runff leaks" + cd tests/runff + bash ff-run.bash || true echo "::endgroup::" diff --git a/TESTING.md b/TESTING.md index 13d0f7f..a4640ee 100644 --- a/TESTING.md +++ b/TESTING.md @@ -133,13 +133,23 @@ ASAN_OPTIONS=detect_leaks=0 ctest --test-dir build-asan --output-on-failure check. Other values are passed straight through โ€” `undefined`, or `address,undefined` for both โ€” but only `address` is currently verified clean. -**`detect_leaks=0` is deliberate, not a workaround.** ForeFire reports zero -ASan *errors* โ€” no use-after-free, no overflow, no double free โ€” on either test -path, which is what makes a blocking job possible. It does leak: nothing owns a -`PropagationModel` (#159), so every one is reported. Leaving leak detection on -would produce a permanently failing job that everyone learns to ignore. The CI -workflow runs the leak check anyway as an informational step, so the number -stays visible, and it can be made blocking once #159 lands. +ForeFire reports zero ASan *errors* โ€” no use-after-free, no overflow, no double +free โ€” on either test path, which is what makes a blocking job possible. + +**Leaks are blocking for the unit suite and informational for `runff`.** The +unit suite reports none: #159 gave the propagation and flux models, the +propagative layer and two orphaned constant-layer arrays an owner, which took +it from about 4.7 MB to zero. Running the suite with leak detection on is +therefore a check rather than a report: + +```bash +ASAN_OPTIONS=detect_leaks=1 ctest --test-dir build-asan --output-on-failure +``` + +`runff` still leaks about 200 kB over 783 allocations, on paths a full +simulation reaches and the unit suite does not, so its leak check runs without +gating the build. Closing those is what would let it join the gate; until then +the figure stays visible in the job log. Note that the sanitizer build writes `bin/forefire` and `lib/libforefireL.so` like any other build, so it replaces a release build in the source tree. diff --git a/src/DataBroker.cpp b/src/DataBroker.cpp index 45e3a79..b67cd5f 100644 --- a/src/DataBroker.cpp +++ b/src/DataBroker.cpp @@ -1062,7 +1062,9 @@ namespace libforefire XYZTDataLayer *layer = new XYZTDataLayer(name, SWCorner, timeOrigin, spatialExtent, Lt, nx, ny, nz, nt, data); - // delete[] data; + // FFArray copies the values into storage of its own, so this + // array is ours to free and nothing refers to it afterwards. + delete[] data; registerLayer(name, layer); } else if (layertype == "flux") @@ -1081,7 +1083,8 @@ namespace libforefire FluxLayer *layer = new FluxLayer(name, atmoSWCorner, atmoNECorner, atmosphericNx, atmosphericNy, domain->getCells(), data, SWCorner, timeOrigin, spatialExtent, Lt, nx, ny, nz, nt); - // delete[] data; + // Copied by FFArray, same as above. + delete[] data; registerFluxModel(newFluxmodel); registerFluxLayer(name, layer); diff --git a/src/FireDomain.cpp b/src/FireDomain.cpp index 269afcd..20f17ec 100644 --- a/src/FireDomain.cpp +++ b/src/FireDomain.cpp @@ -326,13 +326,18 @@ infrontiers.pop_back(); } + // The models this domain registered go back before the broker does: + // they hold a pointer to it, and freeing the broker first would leave + // a destructor reaching through a dangling pointer. + releaseOwnedModels(); + // Deleting data broker and layers if (dataBroker) { delete dataBroker; dataBroker = nullptr; } if (propagativeLayer) { - //delete propagativeLayer; + delete propagativeLayer; propagativeLayer = nullptr; } @@ -695,20 +700,44 @@ params->setParameter(model->getName() + ".keys", properties); //cout<< "loading"<< model->getName()<= NUM_MAX_PROPMODELS ) return false; PropagationModel* model = propModelInstanciation(mindex, mname); if ( model == 0 ) return false; /* Instantiating a flux layer related to this model */ + // Replacing an earlier layer: the old one is this domain's to free, + // and nothing else refers to it. + delete propagativeLayer; propagativeLayer = new PropagativeLayer("ROSlayer", mindex); return true; } @@ -827,6 +856,7 @@ // Otherwise, searching for the model in the available ones size_t mindex = getFreeFluxModelIndex(); + if ( mindex >= NUM_MAX_FLUXMODELS ) return false; string fmname = ""; if ( lname == "heatFlux" ) fmname = "heatFluxBasic"; if ( lname == "vaporFlux" ) fmname = "vaporFluxBasic"; @@ -856,7 +886,16 @@ size_t FireDomain::getFreePropModelIndex(){ size_t mindex = NUM_MAX_PROPMODELS - 1; - while ( propModelsTable[mindex] != 0 ) mindex--; + // Bounded: mindex is unsigned, so walking off the bottom of a full + // table wrapped round to SIZE_MAX and read far out of bounds. + while ( propModelsTable[mindex] != 0 ){ + if ( mindex == 0 ){ + cerr<<"ERROR: no free propagation model slot, max is " + <= NUM_MAX_FLUXMODELS -1){ - cout<<"ERROR No mor flx models allowed, max:"<< NUM_MAX_FLUXMODELS<GetPath(params->getParameter("NetCDFfile")); - - /* initializations for the flux models */ - for ( size_t i = 0; i < NUM_MAX_FLUXMODELS; i++ ) fluxModelsTable[i] = NULL; /* loading the layers for atmospheric variables and coupling variables */ diff --git a/src/FireDomain.h b/src/FireDomain.h index 3debc09..600341b 100644 --- a/src/FireDomain.h +++ b/src/FireDomain.h @@ -309,6 +309,16 @@ class FireDomain: public ForeFireAtom, Visitable { bool addPropagativeLayer(string); size_t getFreePropModelIndex(); + /*! \brief indices this domain put into the shared model tables + * + * propModelsTable and fluxModelsTable are static, so an entry cannot be + * freed just because some domain is going away โ€” only the domain that + * registered it may. These record which ones are ours. */ + std::vector ownedPropModelIndices; + std::vector ownedFluxModelIndices; + /*! \brief deletes this domain's models and clears their table entries */ + void releaseOwnedModels(); + /* Mesh properties */ /*-----------------*/ FFPoint SWLngLat; /*!< SouthWest Corner of the mesh */ diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 18ecabd..7082415 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -9,6 +9,7 @@ add_executable(forefire_unit_tests main.cpp model_sandbox.cpp + test_domain_ownership.cpp test_flux_models.cpp test_http_server.cpp test_model_registry.cpp @@ -25,7 +26,8 @@ endif() # doctest groups its cases into suites; registering one CTest entry per suite # keeps `ctest` output useful without needing doctest's CMake integration. -foreach(_suite "model registry" "propagation models" "flux models" "http server") +foreach(_suite "model registry" "propagation models" "flux models" "http server" + "domain ownership") string(REPLACE " " "_" _suite_id "${_suite}") add_test(NAME "unit.${_suite_id}" COMMAND forefire_unit_tests --test-suite=${_suite} --no-skipped-summary) diff --git a/tests/unit/test_domain_ownership.cpp b/tests/unit/test_domain_ownership.cpp new file mode 100644 index 0000000..8c68a20 --- /dev/null +++ b/tests/unit/test_domain_ownership.cpp @@ -0,0 +1,111 @@ +/** + * @file test_domain_ownership.cpp + * @brief A FireDomain owns what it registers, and only what it registers. + * @copyright Copyright (C) 2025 ForeFire, Fire Team, SPE, CNRS/Universita di Corsica. + * @license This program is free software; See LICENSE file for details. (See LICENSE file). + * + * propModelsTable and fluxModelsTable are static, shared by every domain in + * the process. Two things used to go wrong with that, both covered here (#159): + * + * - Nothing freed a domain's models or its propagative layer, so every + * domain built and thrown away cost about 183 kB, linearly, forever. + * - The constructor wiped the whole table, so building a second domain + * dropped the first one's registrations. The first domain's layer then + * held an index that was empty, and became the *second* domain's model + * once that one registered. + * + * The second is the one worth keeping a test on: it is silent, it produces + * wrong spread rates rather than a crash, and Command.cpp builds exactly that + * second domain for coupled runs. + */ + +#include "doctest/doctest.h" + +#include "model_sandbox.h" + +#include "FireDomain.h" + +#include + +using libforefire::FireDomain; +using libforefire::FFPoint; +using ff_test::ModelSandbox; + +namespace { + +size_t occupiedPropSlots() { + size_t used = 0; + for (size_t i = 0; i < FireDomain::NUM_MAX_PROPMODELS; i++) + if (FireDomain::propModelsTable[i] != 0) used++; + return used; +} + +FireDomain* makeDomain() { + FFPoint sw(0., 0., 0.); + FFPoint ne(1000., 1000., 0.); + return new FireDomain(0., sw, ne); +} + +} /* anonymous namespace */ + +TEST_SUITE("domain ownership") { + +TEST_CASE("a destroyed domain releases its propagation model") { + ModelSandbox sandbox; // holds the fuel table and restores parameters + const size_t before = occupiedPropSlots(); + + FireDomain* domain = makeDomain(); + REQUIRE(domain->addPropagativeLayer("Rothermel")); + CHECK(occupiedPropSlots() == before + 1); + + delete domain; + // Back to where we started: the slot is free and the model is gone. This + // failing means the table fills up and the memory is never returned. + CHECK(occupiedPropSlots() == before); +} + +TEST_CASE("building a domain leaves another domain's models alone") { + ModelSandbox sandbox; + + FireDomain* first = makeDomain(); + REQUIRE(first->addPropagativeLayer("Rothermel")); + + size_t firstIndex = FireDomain::NUM_MAX_PROPMODELS; + for (size_t i = 0; i < FireDomain::NUM_MAX_PROPMODELS; i++) + if (FireDomain::propModelsTable[i] != 0) firstIndex = i; + REQUIRE(firstIndex < FireDomain::NUM_MAX_PROPMODELS); + const libforefire::PropagationModel* firstModel = + FireDomain::propModelsTable[firstIndex]; + + // The constructor used to clear the table here. + FireDomain* second = makeDomain(); + CHECK(FireDomain::propModelsTable[firstIndex] == firstModel); + + // And the second domain must not be handed the slot the first is using. + REQUIRE(second->addPropagativeLayer("Rothermel")); + CHECK(FireDomain::propModelsTable[firstIndex] == firstModel); + + delete second; + // Destroying the second domain must not take the first one's model with + // it: each releases only what it registered. + CHECK(FireDomain::propModelsTable[firstIndex] == firstModel); + + delete first; +} + +TEST_CASE("many domains do not accumulate table entries") { + // The leak was linear and unbounded, so a loop is the shape of the check. + // Slot occupancy standing still is the observable part of it. + ModelSandbox sandbox; + const size_t before = occupiedPropSlots(); + + for (int i = 0; i < 40; i++) { + FireDomain* domain = makeDomain(); + REQUIRE(domain->addPropagativeLayer("Rothermel")); + delete domain; + } + + CHECK(occupiedPropSlots() == before); +} + +} /* TEST_SUITE */ diff --git a/tests/unit/test_model_registry.cpp b/tests/unit/test_model_registry.cpp index f735c5e..3460743 100644 --- a/tests/unit/test_model_registry.cpp +++ b/tests/unit/test_model_registry.cpp @@ -122,46 +122,47 @@ TEST_CASE("every model can be destroyed") { // derived destructor as well is a double free, and this case is what says // so: run it under a build that reintroduces one and it aborts. // - // Nothing deletes a model in a normal run โ€” FireDomain keeps them in - // propModelsTable and fluxModelsTable and never frees either โ€” so these - // destructors are reached only from here. That is precisely why the double - // free survived: the code that trips it does not otherwise run. - ModelSandbox sandbox; - + // The models are freed by ~FireDomain, which is what owns them: it records + // the entries it put in the shared model tables and releases those. So + // each model gets its own sandbox here, and destroying the sandbox is the + // destruction under test. Deleting one by hand instead would be the double + // free this case exists to catch. const std::vector& props = propagationModels(); for (size_t i = 0; i < props.size(); i++) { CAPTURE(props[i]); + ModelSandbox sandbox; PropagationModel* model = sandbox.propagation(props[i]); REQUIRE(model != 0); - delete model; } const std::vector& fluxes = fluxModels(); for (size_t i = 0; i < fluxes.size(); i++) { CAPTURE(fluxes[i]); + ModelSandbox sandbox; FluxModel* model = sandbox.flux(fluxes[i]); REQUIRE(model != 0); - delete model; } } -TEST_CASE("destroying a model does not disturb the next one") { +TEST_CASE("destroying a domain's models does not disturb the next domain") { // A double free often shows up as the *next* allocation coming back // corrupted rather than as an immediate abort, so allocate across the // destruction and check the new model is intact. - ModelSandbox sandbox; - - PropagationModel* first = sandbox.propagation("Rothermel"); - REQUIRE(first != 0); - const std::vector wanted = first->wantedProperties; - const size_t count = first->numProperties; - delete first; + std::vector wanted; + size_t count = 0; + { + ModelSandbox sandbox; + PropagationModel* first = sandbox.propagation("Rothermel"); + REQUIRE(first != 0); + wanted = first->wantedProperties; + count = first->numProperties; + } + ModelSandbox sandbox; PropagationModel* second = sandbox.propagation("Rothermel"); REQUIRE(second != 0); CHECK(second->numProperties == count); CHECK(second->wantedProperties == wanted); - delete second; } TEST_CASE("property registration order is stable within a model") {