Skip to content

Stop freeing the model properties array twice - #157

Merged
antonio-leblanc merged 1 commit into
test/model-unit-suitefrom
fix/model-properties-double-delete
Aug 12, 2026
Merged

Stop freeing the model properties array twice#157
antonio-leblanc merged 1 commit into
test/model-unit-suitefrom
fix/model-properties-double-delete

Conversation

@HugoFara

Copy link
Copy Markdown
Collaborator

Stop freeing the model properties array twice

Stacked on #156 — targets test/model-unit-suite, so the diff below is only this change. Merge #156 first and GitHub will retarget this to dev automatically.

properties is allocated by each model's own constructor with new double[numProperties] and freed by ~ForeFireModel. Sixteen flux models and two propagation models freed it again in their own destructor, so destroying any model that registers at least one property was a double free. The flux ones compounded it by using scalar delete on an array allocated with new[].

Removing the derived deletes and leaving it to the base class is the whole fix. No ownership changes, no signature changes — 18 files, 22 lines deleted.

 /* destructor (shoudn't be modified) */
HeatFluxBasicModel::~HeatFluxBasicModel() {
-	if ( properties != 0 ) delete properties;
}

Why this has never crashed anyone

FireDomain keeps its models in propModelsTable and fluxModelsTable and frees neither, so nothing destroys a model today — every model a simulation instantiates is leaked instead. The bug is unreachable in production and becomes reachable the moment anything does destroy one, including a test.

That leak is still open after this PR. It is a separate change, and fixing it before this one would have turned a silent leak into a crash on every run.

The test comes with the fix

#156 deliberately limited its destructor case to the six models that register no properties, precisely because destroying the others was a double free. That restriction lifts here:

  • every model can be destroyed now runs all 33 models — 17 propagation, 16 flux — instead of two.
  • destroying a model does not disturb the next one allocates a model across a destruction and checks it comes back intact, because a double free often surfaces as the next allocation being corrupted rather than as an immediate abort.

Verified by reintroducing a single delete into ForeFireV1HeatFluxModel and re-running:

double free or corruption (!prev)
test_model_registry.cpp:119: FATAL ERROR: test case CRASHED: SIGABRT
[doctest] test cases: 1 | 0 passed | 1 failed

So the test genuinely holds the fix in place rather than merely passing alongside it. Worth noting the first attempt at that control used heatFluxBasic and passed — it registers no properties, so the != 0 guard made the reintroduced delete a no-op. The bug only bites models that actually allocate.

Verification

  • 22 cases, 2115 assertions, all passing; ctest 3/3.
  • tests/runff unchanged — KML and NetCDF both within tolerance.
  • Negative control above.

AddressSanitizer would have been the natural tool here, but glibc's own heap checking catches this one just as decisively, and the suite needs no special build to do it.

`properties` is allocated by each model's own constructor with
`new double[numProperties]` and freed by ~ForeFireModel. Sixteen flux
models and two propagation models freed it again in their own
destructor, so destroying any model that registers at least one property
was a double free. The flux ones also used scalar `delete` on an array
allocated with `new[]`.
Removing the derived deletes and leaving it to the base class is the
whole fix; no ownership changes.
This has never been hit in production because FireDomain keeps its
models in propModelsTable and fluxModelsTable and frees neither, so
nothing destroys a model today. It is reachable from anything that does,
including a test -- which is why the test that covers it comes with it:
`every model can be destroyed` now runs all 33 models rather than the
six that register no properties, and `destroying a model does not
disturb the next one` allocates across a destruction, since a double
free often surfaces as the next allocation coming back wrong rather than
as an immediate abort.
Verified by reintroducing a single delete: the suite aborts with
"double free or corruption (!prev)" and doctest reports the case as
CRASHED - SIGABRT.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@HugoFara@antonio-leblanc