From d1e8ea50b2808eccf4daa760e6fe6d27ed803479 Mon Sep 17 00:00:00 2001 From: HugoFara Date: Wed, 12 Aug 2026 22:25:35 +0200 Subject: [PATCH] fix(core): give the propagation models and layers an owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Building and destroying a FireDomain with a propagation layer cost about 183 kB every time, linearly and without limit. Two things were missing an owner, and a third made ownership impossible. The propagative layer was never freed: the delete in ~FireDomain was commented out and the pointer nulled, which also removed any chance of a later cleanup finding it. The models were never freed either. propModelsTable and fluxModelsTable are static, shared by every domain in the process, and nothing cleared them. Each domain now records the entries it registered and releases exactly those, before the data broker goes, since the models hold a pointer to it. The reason that could not simply be added: the FireDomain constructor wiped both tables. Building a second domain therefore dropped the first one's registrations, and its PropagativeLayer was left holding an index that was empty until the new domain registered โ€” at which point it resolved to the *second* domain's model. Command.cpp builds exactly that second domain for coupled runs. The wipe is gone; both tables are static storage and already start out null. Two smaller things on the same path: - getFreePropModelIndex counted down through an unsigned index with no lower bound, so a full table wrapped to SIZE_MAX and read far out of bounds. getFreeFluxModelIndex bound-checked but then returned an occupied index, silently overwriting a live model. Both now report and return an out-of-range value that their callers refuse. - DataBroker::addConstantLayer allocated an array, handed it to a layer that copies it, and dropped it. Both delete[] lines were sitting there commented out, one per branch. Measured over 500 domains, each with a Rothermel layer: RSS growth falls from 91,504 kB to 180 kB, and occupied model slots from perpetually climbing to zero after each domain. Under ASan the unit suite goes from 4.7 MB leaked to none, so the leak check is now blocking for it; runff still leaks ~200 kB on paths the suite does not reach and stays informational. tests/unit/test_domain_ownership.cpp covers the release, the cross-domain clobbering and the loop. The model registry tests no longer delete models by hand, which is a double free now that the domain owns them; each takes its own sandbox instead, so destruction is exercised the way it actually happens. Closes #159 --- .github/workflows/sanitizers.yml | 31 ++++---- TESTING.md | 24 ++++-- src/DataBroker.cpp | 7 +- src/FireDomain.cpp | 70 ++++++++++++++--- src/FireDomain.h | 10 +++ tests/unit/CMakeLists.txt | 4 +- tests/unit/test_domain_ownership.cpp | 111 +++++++++++++++++++++++++++ tests/unit/test_model_registry.cpp | 35 +++++---- 8 files changed, 241 insertions(+), 51 deletions(-) create mode 100644 tests/unit/test_domain_ownership.cpp 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") {