Fix use-after-free by making GeneratorParams co-own its Model - #2270
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a use-after-free in the public C API by ensuring GeneratorParams co-owns the Model it was created from, preventing the model-owned Config from being freed while params are still alive.
Changes:
- Add
std::shared_ptr<const Model> model_owner_toGeneratorParamsto extend model lifetime. - Initialize
model_owner_viamodel.shared_from_this()and bindconfigthroughmodel_owner_. - Add a C API regression test covering the destroy-model-then-create-generator sequence.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/c_api_tests.cpp | Adds regression coverage for the C API lifetime/UAF scenario. |
| src/generators.h | Extends GeneratorParams to co-own the model backing its aliased Config. |
| src/generators.cpp | Initializes model_owner_ and binds config through it to prevent UAF. |
jiafatom
force-pushed
the
fix/generatorparams-model-lifetime
branch
from
July 7, 2026 23:06
71a116a to
31a4c64
Compare
GeneratorParams stored only a bare 'const Config&' aliasing the model-owned Config, relying on an undocumented lifetime assumption. Destroying the model (OgaDestroyModel) before OgaCreateGenerator freed the model and its Config, so Generator::Generator's model.shared_from_this() read freed memory (heap-use-after-free, CWE-416). Add a std::shared_ptr<const Model> member to GeneratorParams (declared before the Config reference so it is initialized first) and bind the Config through it. This mirrors Generator, which already co-owns the model. The benchmark-only GeneratorParams(const Config&) path leaves model_owner_ null by design. Add a C API regression test that destroys the model handle before creating the generator and asserts it succeeds without a use-after-free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per reviewer feedback, rename the co-owning member from model_owner_ to model_. The aliased const Config& config member is retained because the benchmark-only GeneratorParams(const Config&) constructor has no Model (model_ is null there), so config cannot be universally derived from model_; keeping the alias also avoids churning the many params.config call sites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jiafatom
force-pushed
the
fix/generatorparams-model-lifetime
branch
from
July 10, 2026 03:14
8284fe9 to
339ef81
Compare
baijumeswani
approved these changes
Jul 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.
Summary
Fixes a use-after-free (CWE-416) reachable through the public C ABI.
GeneratorParamsstored only a bareconst Config&aliasing the model-ownedConfig(src/generators.h), relying on the documented-but-unenforced assumption that "the model outlives the GeneratorParams". It did not take co-ownership of the model. The following ordinary C API sequence therefore triggers a UAF:OgaCreateModel— allocates theModeland itsstd::unique_ptr<Config> config_.OgaCreateGeneratorParams— bindsconfig{*model.config_.get()}, a bare reference; the model's refcount is unchanged.OgaDestroyModel—ExternalReleasedrops the last owner and frees theModeland itsConfig.OgaCreateGenerator(model, params)—Generator::Generatorrunsmodel_{model.shared_from_this()}on the freed model → heap-use-after-free read.Generatoralready co-owns the model viastd::shared_ptr<const Model> model_;GeneratorParamsdid not follow the same discipline, leaving the window open.Fix
src/generators.h: addstd::shared_ptr<const Model> model_owner_toGeneratorParams, declared before theconfigreference so it is initialized first.src/generators.cpp: initializemodel_owner_{model.shared_from_this()}and bindconfig{*model_owner_->config_.get()}. Now the model (and itsConfig) cannot be freed while a params handle is live, and a generator can never be constructed from a freed model. The benchmark-onlyGeneratorParams(const Config&)path leavesmodel_owner_null by design.test/c_api_tests.cpp: addCreateGeneratorAfterDestroyModel, reproducing the PoC ordering (destroy model handle, then create generator) and asserting success without a UAF.Testing
Local build isn't available in my environment (root-owned build dir / needs the jiafa-dev container); relying on CI to build and run the C++ unit tests, ideally under ASan.