Skip to content

Commit da82282

Browse files
committed
fix(g2p): fix destruction order to prevent use-after-free in Task destructors
Task holds a non-owning const ModuleSpec* pointer. ModuleSpec is owned by PackageData and deleted in closeAllLoadedPackages. The previous order (closeAllLoadedPackages then removeAllObjects) left m_spec dangling when Task destructors ran, causing use-after-free that corrupted the heap and later crashed in shared_ptr::_Decref. Fixed by reordering: removeAllObjects (release Tasks while ModuleSpec alive) then closeAllLoadedPackages (delete ModuleSpec) then delete categories.
1 parent 65c7397 commit da82282

1 file changed

Lines changed: 25 additions & 18 deletions

File tree

‎lib/G2P/Core/PackageManager.cpp‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -246,27 +246,34 @@ namespace srt::g2p {
246246
}
247247

248248
PackageManager::Impl::~Impl() {
249-
// Tasks retain non-owning ModuleSpec pointers owned by loaded packages.
250-
// Release every task reference before the corresponding packages.
249+
// Destruction order (matters!):
250+
//
251+
// 1. m_tasks.clear() — release the NO<Task> references held by the
252+
// task index. Tasks are also referenced by ObjectPool::m_objects
253+
// (use_count >= 2), so this only drops the count, not the object.
254+
//
255+
// 2. cate->removeAllObjects() — release the NO<Task>/NO<NamedObject>
256+
// shared_ptrs held by each category's ObjectPool. This is the last
257+
// reference, so Task destructors run HERE. Tasks hold a non-owning
258+
// `const ModuleSpec*` pointer (Task::Impl::m_spec); the pointed-to
259+
// ModuleSpec is owned by PackageData and is still alive at this
260+
// point (step 3 deletes it). Doing removeAllObjects AFTER
261+
// closeAllLoadedPackages would leave m_spec dangling and cause
262+
// use-after-free in Task subclass destructors, corrupting the heap
263+
// and later crashing in shared_ptr::_Decref.
264+
//
265+
// 3. closeAllLoadedPackages() — delete PackageData (which deletes its
266+
// ModuleSpec instances). Categories must still be alive because
267+
// closePackage calls ModuleCategory::loadSpec(Deleted) to remove
268+
// specs from m_modules.
269+
//
270+
// 4. delete cate — destroy the categories themselves.
251271
m_tasks.clear();
252-
// closePackage() invokes ModuleCategory::loadSpec() on each contribute
253-
// to transition ModuleSpec states (Finished/Deleted), so loaded packages
254-
// must be closed while the categories are still alive. Deleting the
255-
// categories first leaves m_categories empty and makes the .at() lookup
256-
// in closePackage throw std::out_of_range during cleanup, which on
257-
// Windows triggers STATUS_STACK_BUFFER_OVERRUN and aborts the process.
258-
closeAllLoadedPackages();
259272
for (const auto &[_, cate] : m_categories) {
260-
// Clear ObjectPool's m_objects before deleting the category.
261-
// m_objects holds NO<NamedObject> (shared_ptr) references to
262-
// drivers/factories/tasks added via addObject(). If these shared_ptrs
263-
// are still alive when ObjectPool::Impl::~Impl() runs (triggered by
264-
// ~ModuleCategory -> ~ObjectPool), their destructor may access
265-
// already-freed memory or trigger double-free if the pointed-to
266-
// objects were released by Runtime shutdown. Clearing m_objects
267-
// here releases the references while the category (and its
268-
// virtual dispatch table) is still intact.
269273
cate->removeAllObjects();
274+
}
275+
closeAllLoadedPackages();
276+
for (const auto &[_, cate] : m_categories) {
270277
delete cate;
271278
}
272279
m_categories.clear();

0 commit comments

Comments
 (0)