Conversation
chapman39
reviewed
Apr 9, 2026
| : dataStore_(dataStore), lifetimeToken_(std::move(lifetimeToken)), primal_(primal) | ||
| { | ||
| } | ||
| DataStore* dataStore_; ///< datastore |
There was a problem hiding this comment.
why not store dataStore_itself as a weak_ptr? which eliminates the use of a raw pointer and may solve this lifetime issue as well.
chapman39
reviewed
Apr 9, 2026
chapman39
left a comment
There was a problem hiding this comment.
i tested this and can confirm it eliminates the heap use after free issue
15 tasks
chapman39
approved these changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The original bug was a teardown use-after-free.
A
Stateobject keeps a raw pointer to itsDataStore. Normally that is fine because theDataStoreis still alive when theStatedestructor runs. But ASan found a case where a copiedStateoutlived the last owner of theDataStore. When that late-destroyedStateran its destructor, it called back intoDataStore::try_to_free()through a dangling pointer. That is the crash.The first fix added a lifetime token so a
Statecould tell whether itsDataStorewas already gone. That part was correct. But the first version also changed the destructor path so it stopped callingtry_to_free()in normal cases. That avoided the crash, but it also broke one of gretl's important behaviors: freeing state memory once a step is inactive and no external handles are keeping it alive.The final fix keeps the lifetime token, but narrows the logic:
Stateis destroyed or reassigned, it first saves the old datastore pointer, old step, and old lifetime tokenStatetry_to_free()if the saved token says the oldDataStoreis still aliveThis gives both required behaviors:
DataStoreis already destroyed, theStatedestructor does nothing dangerousDataStoreis still alive, normaltry_to_free()logic still runs, so gretl can evict inactive state data from memoryThe added tests check both sides:
DataStore