lsan: stop suppressing the frames user code runs under; fix the leaks that hid behind them - #38385
lsan: stop suppressing the frames user code runs under; fix the leaks that hid behind them#38385robobun wants to merge 1 commit into
Conversation
… that hid behind them
An LSan suppression matches any frame of the allocation stack, so
leak:Bun::evaluateCommonJSModuleOnce and
leak:JSC::JSModuleLoader::evaluateNonVirtual in test/leaksan.supp hid
every leak made by code running at a module's top level (-e scripts,
entries, require()d and imported modules, worker eval scripts, test
files), which is where most fixtures start their work.
JSC__JSModuleLoader__loadAndEvaluateModule names the same kind of frame.
Remove all three. The microtask-tick hook installed by
Zig__GlobalObject__create was a lambda, so its symbol carried that name
and leak:Zig__GlobalObject__create hid everything that ran from a
nextTick or promise continuation on the main thread too; install the
identical named function that already existed instead.
Running every LSan-validated test file with the trimmed file surfaced
what those entries were hiding:
- process.on("memoryPressure") left registered at exit leaked the
watcher box: its RareData slot is an erased pointer and nothing
disarmed it at teardown. Disarm it in the teardown stop phase.
- crypto.Certificate.exportChallenge() leaked the buffer returned by
ASN1_STRING_to_UTF8 on every call. ncrypto::ExportChallenge returns an
owning DataPointer, as upstream ncrypto does.
- process.dlopen()'s file name for a napi env that an undeleted napi_ref
keeps alive forever was never freed, since ~NapiEnv never runs. Free it
at the end of the env's cleanup, like its hook set.
- A Bun.connect() whose resolve result was already queued when its VM
went away leaked the us_connecting_socket_t: closing it could not
cancel the queued completion, and the tick that would have run it never
came. The teardown drain now runs the dns-ready queue first.
- test/regression/issue/26249.test.ts's fixtures never close()d their
cc() library, which stays loaded by design; close them.
- broadcast-channel-worker-gc.test.ts (EventNames per worker, #38164, and
ShadowRealm console objects) and 30205.test.ts (--parallel workers skip
napi env cleanup) report leaks owned elsewhere; listed in
no-validate-leaksan.txt with the reason.
bun:internal-for-testing gains lsanIntentionalLeak() (ASAN builds only)
so test/internal/leaksan-suppressions.test.ts can check that a leak made
from each of those contexts is reported with test/leaksan.supp applied.
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (20)
Comment |
|
Status: reproduced and fixed; waiting on CI. Reproduced on a debug ASAN build of main (e697804) with the CI leak settings: a worker calling The survey of the full LSan-validated test set that drove the list of fixes is in the PR description. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it removes broad LSan suppressions and fixes four separate leaks across native teardown paths (napi env cleanup, uws DNS drain, memory-pressure watcher, ncrypto), a human look would still be worthwhile.
What was reviewed:
- Verified
checkIfNextTickWasCalledDuringMicrotaskmatches the removed lambda body and is already used byresetOnEachMicrotaskTick. - Verified
us_internal_socket_after_resolveshort-circuits onc->closedand thatus_socket_group_close_all_excloses every connecting socket beforedrain_closed_socketsruns, so the new DNS drain frees rather than reconnects. - Verified
DataPointer::~DataPointerusesOPENSSL_clear_free, matchingASN1_STRING_to_UTF8's allocator; the size-0 case still frees on scope exit. - Verified
SweepResult::andsemantics and thatstop_for_vm_teardownavoids allocatingRareDatawhen the watcher was never armed.
Extended reasoning...
Overview
This PR removes three overly-broad LSan suppression entries (Bun::evaluateCommonJSModuleOnce, JSC::JSModuleLoader::evaluateNonVirtual, JSC__JSModuleLoader__loadAndEvaluateModule) that sat on the call stack of all top-level user code and were therefore hiding every leak fixtures produced. It also swaps a lambda in Zig__GlobalObject__create for the pre-existing named checkIfNextTickWasCalledDuringMicrotask so that the retained leak:Zig__GlobalObject__create entry stops matching microtask/nextTick frames. To make removal viable, it fixes the four real leaks the survey uncovered: the MemoryPressureWatcher box at VM teardown, the ASN1_STRING_to_UTF8 buffer in exportChallenge, the napi env filename when ~NapiEnv never runs, and the us_connecting_socket_t whose queued DNS completion outlives the loop's last tick. It adds an lsanIntentionalLeak() test hook, a suppressions regression test, and per-fix leak tests, plus two no-validate-leaksan.txt entries with tracked reasons.
Security risks
None identified. The crypto change is a mechanical ownership fix (unowned Buffer<char> → owning DataPointer) that matches upstream ncrypto and preserves the copy into a JSC ArrayBuffer. No auth, permissions, or input-validation surface changes.
Level of scrutiny
High. Per the repo's own review rules, native memory safety is the most-blocked category, and this touches it in four independent subsystems plus VM teardown ordering. Each fix individually looks correct on inspection: the ncrypto change matches upstream and DataPointer's destructor uses the right free; NapiEnv::cleanup() freeing filename mirrors the existing m_cleanupHooks release and ~NapiEnv still handles it via std::exchange; us_internal_handle_dns_results at teardown is safe because close_all_socket_groups has already set c->closed on every connecting socket, so after_resolve takes the free-only branch; the memory-pressure teardown reads the raw rare_data option to avoid lazy allocation and reuses the existing uninstall. The ZigGlobalObject lambda→named-function swap is behaviorally identical. But the aggregate is 20 files spanning C++, Rust, and CI configuration, and the correctness of the uws change depends on a teardown ordering invariant that a maintainer familiar with stop_phase_sweep should confirm.
Other factors
The PR description is unusually thorough — it documents a full-suite survey, per-fix fails-without/passes-with verification, and Windows cross-check. Tests follow the harness conventions (subprocess + LSan env, skipIf(!isASAN), concurrent drain of stdout/stderr/exited, per-test 90s timeout with a stated reason). The two new no-validate-leaksan.txt entries are documented with issue references rather than silently suppressed. Nothing blocks; the deferral is purely because the scope and subsystem breadth exceed what an automated approval should cover.
|
Updated 4:40 AM PT - Aug 14th, 2026
❌ @robobun, your commit 3e95858 has 6 failures in
🧪 To try this PR locally: bunx bun-pr 38385That installs a local version of the PR into your bun-38385 --bun |
1 participant
Problem
test/leaksan.supphadleak:Bun::evaluateCommonJSModuleOnce,leak:JSC::JSModuleLoader::evaluateNonVirtualandleak:JSC__JSModuleLoader__loadAndEvaluateModule, all from the original bulk list in ci: instrument being able to run leaksanitizer #21142, undocumented.-escript, a CJS or ESM entry, arequire()d or imported module, a workerevalscript or a test file was silently suppressed, and most fixtures start their work at top level. Found while fixing Bun.write: carry the promise as the WriteFile job's JS side so a stopped worker releases it #38332: aBun.write()leak at a worker's top level went unreported, and moving the same call intosetImmediatemade LSan report it. worker: free the thread's event name table when the worker thread exits #38164 ran into the same thing independently.leak:Zig__GlobalObject__createhad the same effect on the main thread for everything that runs from a microtask orprocess.nextTick: the microtask-tick hook installed inZig__GlobalObject__createwas a lambda, so its symbol isZig__GlobalObject__create::$_0::operator(), and that frame is on the stack of ESM evaluation,awaitcontinuations and nextTick callbacks (found by the new test;LSAN_OPTIONS=print_suppressions=1attributes such leaks to that entry).process.on("memoryPressure", ...)left registered at exit leaks theMemoryPressureWatcherbox (memory_pressure.rs): theRareDataslot is an erased pointer thatRareData's drop does not free, and nothing disarms the watcher at teardown.crypto.Certificate.exportChallenge()leaks the bufferASN1_STRING_to_UTF8allocates on every call:ncrypto::ExportChallengereturned it as an unownedBuffer<char>andjsCertExportChallengecopied it without freeing it (test-crypto-certificate.js).process.dlopen()builds afile://name for each napi env (BunProcess.cpp,toFileURI) that only~NapiEnvfrees. Anapi_refthe addon never deletes (the usual constructor reference, or anapi_wrapresult) holds aRefto the env, so~NapiEnvnever runs and the name leaks for every such addon (9 napi test files, vitest's addon,30205.test.ts).Bun.connect()to a host with several addresses allocates aus_connecting_socket_t; if its resolve completion is already queued on the loop when the VM is torn down,us_connecting_socket_closecannot cancel it and leaves the struct tous_internal_socket_after_resolveon the next tick, which never comes (worker_destruction.test.ts, 50 of 50 workers).Fix
Zig__GlobalObject__createinstallscheckIfNextTickWasCalledDuringMicrotask, the named function a few lines up whose body was identical to the lambda, so the hook's frame no longer carries the creation function's name andleak:Zig__GlobalObject__createis back to covering only what creating a global allocates.stop_active_handles_for_vm_teardownalso disarms the watcher. That hook runs in the teardown stop phase, whileRareData.file_pollsand the loop are alive, whichuninstallneeds to unregister the poll (and on Windows to join its thread); the dns channel and the other per-VM handles are stopped at the same point. It reportsStoppedonly when it found a watcher, so the sweep's second pass is still idle.uninstalltakes the VM so the listener-removal path and teardown share it.ExportChallengereturns an owningDataPointer, what upstream ncrypto returns; it frees withOPENSSL_clear_free, matching theOPENSSL_mallocinsideASN1_STRING_to_UTF8, and the binding's copy is the only thing that outlives the call. An empty challenge's 1-byte buffer, which the oldlen == 0return dropped too, is freed the same way.NapiEnv::cleanup()freesfilenameat its end. Same situation and same place as them_cleanupHooksrelease already in that function (~NapiEnvmay never run); nothing uses the env after cleanup, andnode_api_get_module_file_namealready answers""for a null name.~NapiEnvstill frees it for envs that do die.Loop::drain_closed_sockets, which teardown already calls after closing every group (stop phase) and again after the JSC VM is gone, runsus_internal_handle_dns_resultsfirst. Every connecting socket still on that queue has been closed by then, soafter_resolveonly releases the cache request and moves it toclosed_connecting_head, which the existing drain frees; nothing can start a connection from there. The same race on the main thread was only invisible because the never-freed loop still pointed at the struct.26249.test.ts's fixtures close theircc()library: an unclosed library stays loaded by design (FFI::finalize), andcc.test.tsis already excluded from LSan for that reason (bun:ffi: free the FFI bookkeeping on GC when close() was not called #36070 would change this).broadcast-channel-worker-gc.test.tsand30205.test.tsgo intotest/no-validate-leaksan.txtwith their reasons: the first reports the per-workerEventNamestable (worker: free the thread's event name table when the worker thread exits #38164) and a ShadowRealmConsoleObjectleak, the second a napi env thatbun test --parallelworkers never clean up; both reported separately. A suppression would not do here: the allocation frames involved (toFileURI,us_socket_group_connect, ...) are the same ones the fixes above are tested through.lsanIntentionalLeak()inbun:internal-for-testing(ASAN builds only) leaks one malloc'd block attributed tojsFunction_lsanIntentionalLeak, so the suppressions file can be tested against a known leak.Zig::SourceProvider::create,Bun__transpileFile,Zig::GlobalObject::moduleLoaderResolve,Zig::ImportMetaObject::*, theJSC::Parserentries). JSC evaluates the entry module from internal microtasks, soJSC__JSModuleLoader__loadAndEvaluateModuleonly ever covered the synchronous resolve and fetch of the entry; the survey found nothing attributed to it, and it names the same kind of frame as the other two.test/internal/leaksan-suppressions.test.ts(new): a leak from a-escript, CJS entry, ESM entry,require()d module, imported module, worker eval script, top level of abun testfile, a nextTick callback, anawaitcontinuation, and asetImmediatecallback as control, each of which must be reported withtest/leaksan.suppapplied. With the three entries put back the seven module rows fail; before the hook change the two main-thread ESM rows failed with the trimmed file as well.process-memory-pressure.test.ts,node-crypto.test.js,napi-env-cleanup-leak.test.ts(new file, so it does not depend on the rest ofnapi.test.ts, two of whose cases time out locally),worker-shutdown-post-leak.test.ts: one leak-checked case each, using the fixture the survey found or a reduction of it (the connect case seeds the DNS cache with two addresses throughdnsCacheSeed, so the queued completion is deterministic). Withsrc/stashed and rebuilt each of these fails with the leak named above; with the fix all pass, as do the whole files.test-crypto-certificate.js,worker_destruction.test.ts, the napi files,vitest.test.ts,26249.test.ts) run clean with the final build and file.cargo check --target x86_64-pc-windows-msvcforbun_runtimeandbun_uws_sys(the memoryPressure and loop changes have Windows halves).worker-shutdown-post-leak.test.ts's existing case gets the same 90 s timeout as the new one (it runs in 4 to 5 s here against a 5 s default); test: give worker-shutdown-post-leak an explicit 90s timeout #38157 and worker: free the thread's event name table when the worker thread exits #38164 make the same edit, whichever lands second has a one-line conflict.Background
detect_leaks=1andLSAN_OPTIONS=suppressions=test/leaksan.supp(scripts/runner.node.mjs), andbunEnvpasses those variables on, so every bun process a test spawns is checked too; a report fails the test.BUN_DESTRUCT_VM_ON_EXIT=1, also set by the runner, makes exit run the VM teardown so that what the VM owns is freed rather than reported.leak:patternline suppresses a leak ifpatternis a substring of any recorded frame's symbol;malloc_context_size=30bounds how many frames are recorded. The release build records more meaningful frames per stack than the debug build (less wrapper depth), so anything the debug build attributes to one of these entries is at least as hidden in CI.Bun::evaluateCommonJSModuleOncecalls a CommonJS module's wrapper function (also the-eand workerevalpath);JSC::JSModuleLoader::evaluateNonVirtualruns an ES module graph's bodies; the on-each-microtask-tick hook is a VM callback JSC invokes between microtasks, which Bun uses to drain the nextTick queue, so on the main thread the microtask queue (module evaluation included) is often drained from inside it.VirtualMachine::stop_phase_sweep): before the JSC VM is destroyed, every native handle is stopped, repeating until a sweep finds nothing;RareDatais the per-VM bag of lazily created subsystems, holding the ones whose types live in higher crates as erased pointers that the owning crate must free.NapiEnvandNapiRefare allocated by bmalloc's typed heaps, which LSan neither reports nor scans in this build, so a libc allocation owned only by such an object shows up as a direct leak, and aMalloc=1test makes all of that memory visible (the two excluded tests use it).Survey: what the trimmed file reports across the LSan-validated test set
Method: the 5603 files
scripts/runner.node.mjswould validate (everything minustest/no-validate-leaksan.txt) were run with the debug ASAN build,BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1,malloc_context_size=30as in CI, andLSAN_OPTIONS=log_path=...:exitcode=0, collecting every report from the test process and all children without changing test outcomes. 29 files hit the survey's 3 minute limit and were not leak-checked to completion. Each of the 23 files that produced a report was then re-run with the old file andprint_suppressions=1to see which entry, if any, had been hiding it. This was done before the hook change; what that additionally exposes is being checked by CI on this PR.process-memory-pressure.test.tsBun::evaluateCommonJSModuleOnce(fixed)node/test/parallel/test-crypto-certificate.jsExportChallengebuffer, 2 x 20 BBun::evaluateCommonJSModuleOnce(fixed)node-napi-testsfiles,napi.test.ts(4 children),vitest.test.ts,30205.test.tstoFileURIname, ~100 B per envevaluateCommonJSModuleOnce/evaluateNonVirtual(fixed;30205's--parallelchildren additionally skip env cleanup, excluded)worker_threads/worker_destruction.test.tsus_connecting_socket_t, 104 B x 50evaluateNonVirtual(fixed)regression/issue/26249.test.tsFFIstruct + symbol tables of an unclosedcc()libraryevaluateNonVirtual(fixtures close the library)broadcast-channel-worker-gc.test.tsMalloc=1: 24EventNamestables, ShadowRealmConsoleObjects and their atomsevaluateNonVirtual(#38164 + handed off; excluded meanwhile)macro-test.test.tsnode:fsBindingbox in thebun buildmacro VM (#35159)Bun::generateModule; identical with both files at this stack depthcli/inspect/test-reporter.test.tsBun.stderrblob of the inspector's own globalDebugger::start_js_debugger_threadentry; identical with both filesbun/plugin/plugins.test.tsPluginRunner::on_resolveleaks on purposeBun__transpileFile; identicaltest-no-addons-resolution-condition.jsModuleBufsscratch of exited threadsBun__resolveSync; identicalregression/issue/26851.test.tsExpect/ScopeFunctions/ParsedSourceMapobjectscli/hot/watch-many-dirs.test.tscreate-jsx.test.ts,spawn-pipe-leak.test.tsFilePollblocks of processes the survey killed on timeoutEntries that still sit above some user code, left alone because nothing in the suite needed them removed:
Bun::generateModule(builtin module bodies),Bun__transpileFile(macros andonLoadplugins run under it),JSC::arrayProtoFuncSort/stringProtoFuncReplaceUsingRegExp/jsonProtoFuncStringify(user callbacks),jsHTTPParser_execute.JSC::moduleLoaderParseModuleandJSC::moduleLoaderModuleDeclarationInstantiationno longer exist in the vendored JSC. Many Zig-era entries (runtime.*,jsc.*) no longer match any symbol either.