Uh oh!
There was an error while loading. Please reload this page.
Fix borrowed module dictionary reference and PyState exit-time use-after-free in the export wrapper - #259
Open
astherath wants to merge 2 commits into
Conversation
PyModule_GetDict() returns a borrowed reference, but findClass() Py_DECREF'd it once per instantiation (and once more on the compile- error path). Each fmi2Instantiate of the same FMU therefore removed a reference the wrapper never owned; the module dictionary is eventually released while the module and its functions still point at it. Observable from the host: sys.getrefcount(module.__dict__) falls below len(gc.get_referrers(module.__dict__)) by one per instantiation. The compile-error path also called Py_Finalize() inside a shared library that does not own the interpreter (a Python host aborts with "_Py_GetConfig: ... thread state is NULL"), printed and cleared the exception so the caller could not report it, and Py_DECREF'd a NULL pCode. Stop decrementing the borrowed dictionary, leave interpreter finalization to the host, and leave the compile error set so that fmi2Instantiate fails with fmi2Fatal instead of crashing the process. Adds test_integration_reinstantiate_same_fmu.
The namespace-scope std::shared_ptr<PyState> was released twice at process exit: by its __cxa_atexit-registered destructor and again by finalizePythonInterpreter() from the library unload hook, which _dl_fini runs afterwards. libstdc++ does not null the control-block pointer on destruction, so the second release touched freed memory. Hosts saw an intermittent "corrupted double-linked list" abort on exit; AddressSanitizer reports the heap-use-after-free on every exit, including a plain fmpy.simulate_fmu() run. Keep the holder heap-allocated for the life of the process (no static destructor) and reset it under pyStateMutex from the unload hook, which stays idempotent. Also hand the shared PyState to the slave *before* constructing it: the assignment after std::make_unique was a dead store since NTNU-IHB#237 (data is passed by value), so slaves never actually co-owned PyState. They now do, as NTNU-IHB#211/NTNU-IHB#213 intended, so the interpreter cannot be finalized under a live slave.
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 freeto 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
findClass()released a borrowed module-dictionary reference and mishandled compile errors.PyStateafter its static destructor had already freed the shared-pointer control block.Bug 1: borrowed module dictionary
On master,
findClass()storesPyModule_GetDict(pyModule)atsrc/pythonfmu/PySlaveInstance.cpp:57. The result is borrowed, but mastercalls
Py_DECREF(pGlobals)on the normal path at line 116 and again in thecompile-error path at lines 64–72. Each instantiation therefore produces a
one-reference refcount drift:
sys.getrefcount(module.__dict__)falls belowlen(gc.get_referrers(module.__dict__)), while the module and its functionsstill refer to that dictionary.
The same compile-error path calls
Py_Finalize()from a shared library thatdoes not own the interpreter. A Python host aborts with
_Py_GetConfigand aNULL thread state instead of reporting the failed instantiation.
The fix removes both borrowed-reference decrements and keeps the existing
Py_Finalize()removal. It also removesPyErr_Print()and the NULLPy_DECREF(pCode), leaving the exception set so the caller reportsfmi2Fataland FMPy raisesFailed to instantiate model.Bug 2:
PyStateexit lifetimeMaster has a namespace-scope
std::shared_ptr<PyState>at line 667. Its__cxa_atexitstatic destructor runs before_dl_finiinvokes the platformunload hook, which calls
finalizePythonInterpreter()at line 695.libstdc++ leaves the shared-pointer control-block pointer (
_M_pi) danglingafter the first release, so the second release is a heap-use-after-free.
On glibc, the DSO is
NODELETEbecause it exportsSTB_GNU_UNIQUEsymbols, so FMPy's
freeInstance()/dlclose()leaves it mapped and the hookruns at process exit. This affects every Linux FMPy run, including one plain
fmpy.simulate_fmu()and upstream's own integration tests under ASan; it doesnot require multiple FMUs.
The fix heap-allocates the shared-pointer holder for process lifetime and
resets its contained pointer under
pyStateMutexfrom the unload hook. Thereset is idempotent.
createInstance()now returns the shared-pointer copytaken under the mutex and assigns it to
data.pyStatebefore constructing theslave. This restores the intended co-ownership: a live slave keeps
PyStatealive, so a non-Python host cannot finalize it underneath that slave.
On Windows, please confirm that the anonymous-namespace
DllMainis invoked;dumpbin /symbols PySlaveInstance.obj | findstr DllMainwill show a mangled?DllMain@?A0x...if it is not the global entry point. I can moveDllMaintoglobal
extern "C"scope in a follow-up if needed.How to reproduce
Build the wrapper and a test FMU with ordinary tools (the installed
pythonfmubinary is used by default;
--wrapperinjects a separately built library):Bug 1: dict_referrers.py
The master build exits non-zero (the mismatch appears by the second
instantiation); the fixed build exits zero.
Bug 2: exit_simulate.py
The deterministic command is:
The master wrapper reports
heap-use-after-freeinfinalizePythonInterpreter; the fixed wrapper exits cleanly. The plainallocator abort is heap-layout dependent, so ASan is the required detector.
Bug 1 compile-error path: compile_error_path.py
Master aborts with status 134. With the borrowed-reference fix, FMPy raises
Failed to instantiate model, printsHOST_STILL_ALIVE, and exits 0.Verification
-Wall -Wextra: master and branch each emit 16 pre-existing warnings; normalized warning sets are identical (delta 0).1270 passed, 1 failed; the sole failure is the pre-existingtest_default_experimentFMPy 0.3.31 string-versus-number assertion. The new regression test passes.1 failed, 10 deselected); against this branch it passes (1 passed, 10 deselected).-k 'not throw_py_error'):10 passed, 1 deselected, with no sanitizer report.exit_simulate.py: master reports the finalize-time heap-use-after-free; branch printsSIMULATED rows=1001 last_x=0.500and exits 0.Known remaining issues not addressed here
The following pre-existing issues are follow-up work: leaked MRO/reflection temporaries and duplicate
sys.pathentries; leaked logger and FMU-state temporaries; missing NULL checks; double cleanup incleanPyObject()and failure cleanup ininitialize(); an uninitializedpClass_; and signed/unsigned value-reference warnings. These are intentionally outside this minimal fix.These issues were found while hosting many FMUs in one process with FMPy.