Repository navigation
fix(ep-plugin): keep the plugin library mapped after ORT unregisters it - #1987
Conversation
`RegisterExecutionProviderLibrary` dlopens the plugin cdylib and `UnregisterExecutionProviderLibrary` drops that reference, after which the loader is free to unmap its text. Nothing withdrew the pointers this library publishes into the process first: * `status::set_host_api` caches the host `OrtApi` in a process-global that lives in this library's data segment; * every `thread_local!` with drop glue that a kernel touches registers a destructor whose function pointer is in this library's text, against whichever thread ran the kernel -- including ORT's own intra-op threads, which we neither own nor join; * instrumented builds register a profile writer at load and run it at process exit, also from this library's text. Take one never-released reference to our own module on the way in, next to the global that motivates it, so the mapping outlives ORT's reference. `init_host_api` is the single choke point every entry point reaches, so the macro, the CPU plugin's hand-written `CreateEpFactories` and the shared-EP path are all covered without a third lockstep site. Measured on Linux, three runs each, before the pin: a bare register -> unregister unmaps the cdylib outright (4 mapping entries -> 0), while the same sequence after a real `Run` retains all 4, because running a kernel pins the DSO on glibc as a side effect. So whether the window is open depended on whether a kernel happened to run first -- an accident of the loader, not a property. Windows `FreeLibrary` grants no such reprieve, which is the standing hypothesis for the `0xC0000005` at process exit on the `Rust coverage (Windows x86_64)` lane (#1672, #983): all tests report ok, then the process faults during teardown. The new test asserts the no-`Run` arm specifically, since it is the only arm on Linux where the pin is load-bearing; a version with a `Run` in it would pass with or without this change. Verified by mutation: dropping the `pin_plugin_library()` call fails it 3/3 with 0 entries, and dropping `RTLD_LAZY` from the dlopen mode fails it too (glibc rejects a mode with neither `RTLD_LAZY` nor `RTLD_NOW`) -- a real bug this test caught. Also corrects the `ORT_EP_LOCK` comment, which described the lock as keeping the register/unregister cycle count below the threshold of a use-after-free. That bug was fixed in c92838d and is now guarded by a deliberate 25-cycle stress test, and serialising tests cannot lower a per-process cycle count in any case -- it only orders the same cycles. Anyone triaging #1672 from that comment is sent to a mechanism the same file's own evidence block rules out. Refs #1672, #983 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1987 +/- ##
==========================================
+ Coverage 80.25% 81.04% +0.79%
==========================================
Files 408 425 +17
Lines 189125 208146 +19021
Branches 189125 208146 +19021
==========================================
+ Hits 151773 168683 +16910
- Misses 31941 33820 +1879
- Partials 5411 5643 +232
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…namicallyLoaded is silent Both raised by independent adversarial review of the pin: neither is a defect, but each was an unstated deliberate choice, which is how a deliberate choice later becomes an unnoticed hole. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Independent adversarial review (Opus), and what it changedRan a separate reviewer against this branch with instructions to falsify, not to praise. It independently reproduced both mutations rather than taking my table on trust — removing the No serious findings. Two things it verified that I had asserted but not proven:
Two minor findings addressed in
Limitations it named that I am accepting rather than fixing, so they are on the record:
|
|
Verified this on a Windows host before merging, since Windows is the platform the PR argues is worse. The fix is genuinely cross-platform. One observation worth recording, not a blocker. The 182-line regression test carries a target-level
The gating itself is defensible: the test reads
So the regression guard covers the platform where the bug is least likely to bite, and does not cover the one where it actually bit. The fix is right and I am merging it; a Windows-side guard — even just asserting |
RegisterExecutionProviderLibrarydlopens the plugin cdylib andUnregisterExecutionProviderLibrarydrops that reference, after which the loader is free to unmap its text. Nothing withdraws the pointers this library has already published into the process:status::set_host_apicaches the hostOrtApiin a process-global that lives in this library's data segment;thread_local!with drop glue that a kernel touches registers a destructor whose function pointer is in this library's text, against whichever thread ran the kernel — including ORT's own intra-op threads, which we neither own nor join;Each is a function pointer into a hole once the library is unmapped.
The measurement
Linux, three runs each, on
mainbefore this change:Run→ unregisterRunning a kernel pins the DSO as a side effect: a
thread_local!with drop glue registers a destructor against the object and glibc will not unload an object with pending TLS destructors. So on Linux, whether the unmap window opens depends on whether a kernel happened to run first — an accident of the loader, not a property this library establishes. WindowsFreeLibrarygrants no such reprieve.That is the standing hypothesis for #1672 / #983:
plugin_ort_e2eonRust coverage (Windows x86_64)reports every testokand then exits0xC0000005. Under coverage the instrumented cdylib registers a profile writer that runs at process exit; each test unregisters the library. I cannot test Windows locally, so this is a hypothesis with a Linux-verified mechanism, not a proven fix — the Windows coverage lane is its falsifier.The fix
One never-released reference to our own module, taken on the way in, next to the global that motivates it.
init_host_apiis the single choke point every entry point reaches, soexport_ep_factories!, the CPU plugin's hand-writtenCreateEpFactoriesand the shared-EP path are all covered without adding a third lockstep site to a file that already warns about lockstep drift.dladdr→dlopen(RTLD_NOLOAD | RTLD_NODELETE | RTLD_LAZY), handle deliberately neverdlclosed.GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN | ..._FROM_ADDRESS).No new packages enter the build —
libcandwindows-sys 0.61are already in the lockfile; only two dependency edges are added.Why the test has no
Runin itDeliberately. The no-
Runarm is the only arm on Linux where the pin is load-bearing; a test that ran a model would pass with or without this change and assert glibc's accident instead of our property. The module doc says so, so a later "simplification" cannot silently make it vacuous.Verified by mutation, not by assumption:
pin_plugin_library()call4 mapping entries -> 0RTLD_LAZYfrom the dlopen modeinvalid mode for dlopen()The second is a bug this test actually caught: POSIX requires exactly one of
RTLD_LAZY/RTLD_NOWin every mode, and my first version had neither, so the pin silently reportedFailedwhile everything still passed. The instrument found it in one run.PinOutcome::NotDynamicallyLoadedexists for the same reason: unit tests link the crate into an executable, wheredlopenof the main program fails on glibc. Detecting that case explicitly keeps a benign failure from masking a real one.Also in this PR
The
ORT_EP_LOCKcomment inplugin_ort_e2e.rsdocumented the lock as keeping the register/unregister cycle count below the threshold of a use-after-free at cycle ≥6. That bug was fixed inc92838dbaand is now guarded by a deliberate 25-cycle stress test — and serialising tests cannot lower a per-process cycle count in any case, it only orders the same cycles. The same file's own evidence block concludes "cycle count is therefore not the variable; library load/unload is". Anyone triaging #1672 from that comment is sent to a mechanism the file itself rules out; I was, for a while.Validation
cargo test -p onnx-runtime-ep-cpu-plugin -p onnx-runtime-ep-plugingreen on the rebased tree, includingplugin_ort_e2e56/56 andstress_register_run_unregister_cycles— the pin does not disturb register/unregister cycling.cargo clippy --all-targetsclean on both crates;cargo fmtclean.cargo docerror count unchanged at 21 (all pre-existing, none in the touched files).Refs #1672, #983