Skip to content

test(aot): name a leak record by the function that allocated - #48461

Open
robobun wants to merge 3 commits into
mainfrom
robobun/52389057/name-leak-records-by-function-after-aot-rewrite
Open

robobun wants to merge 3 commits into
mainfrom
robobun/52389057/name-leak-records-by-function-after-aot-rewrite

Conversation

@robobun

@robobun robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • test/aot/no-leak-records-at-exit.test.ts names a leak record by its first frame that is no allocator. An executable passes when bun run has each of its names.
  • On main each Malloc=1 record has the name bmalloc::SystemHeap::malloc, because isAllocator() does not know bmalloc::. Each Rust record has the name alloc, the last word of the inlined std::sys::alloc::unix::alloc.
  • So an executable-only record passes, also after AOT: front end, test runs and tests for the rewritten compiler #47942.

Fix

  • A record now has the name of the first called function that is neither allocator nor container. getCalledFunctions() leaves out inlined functions.
  • bmalloc::, __rustc::, bun_alloc::, OPENSSL_* and a class operator new are allocators. A Rust method is a container frame when the language, hashbrown or smallvec has its code.
  • A name has no requires clause, qualifier or location, and an operator keeps its < or >. A stack that ends in containers gives its last function.
  • Verified: test/aot/no-leak-records-at-exit.test.ts, 31 pass on a release ASAN build (89452d2). A probe with four executable-only records passes on main and fails here.

Background

  • A leak record is one allocation site of a LeakSanitizer report.
  • With debug information a symbolizer also writes each inlined function. A release build names an inlined Rust function by its last word.
  • Rust writes a method as <Type as Trait>::method. The crate of the trait or of the type has its code.
  • This is the file of test(aot): name a leak record by the function that allocated #47443 at ed7a365, with what review asked for here.
  • Considered: keep the inlined frames and add alloc to the allocators. Any function of that name then hides a record.
Notes

What main does (98cda0d, after #47942 and #48120, release ASAN build)

Names that getLeakRecords() gives:

program main this branch
workers that end, Malloc=1 bmalloc::SystemHeap::malloc for all 8 records of the executable and all 20 of bun run executable: WebCore::eventNames, WebCore::EventNames::EventNames, Zig::GlobalObject::setConsole. bun run: those three and JSC::Identifier::add, JSC::CommonIdentifiers::CommonIdentifiers, WebCore::BunBuiltinNames::BunBuiltinNames
workers that serve their default export, no Malloc alloc <bun_runtime::server::NewServer<false, true>>::init

The records and their counts are the same as before #47942.

The probe

In the test body, give bun run the program console.log("hello", 1 + 2) in place of entry.ts, and run -t "workers that end". bun run then has 2 records and the executable 8. Four of them are of WebCore::eventNames and WebCore::EventNames::EventNames, which bun run of that program does not have.

  • main: pass.
  • This branch: fail, with those four records and no other record.

How it was run

bun scripts/build.ts --profile=release-asan --build-dir=build/release-asan -j4
B=build/release-asan/bun-asan
id=$(readelf -n $B | sed -n 's/.*Build ID: //p')
mkdir -p /tmp/dbg/.build-id/${id:0:2}
llvm-objcopy --only-keep-debug --decompress-debug-sections $B /tmp/dbg/.build-id/${id:0:2}/${id:2}.debug
llvm-strip --strip-debug -o $B-stripped $B
ASAN_SYMBOLIZER_PATH=$(which llvm-symbolizer) LLVM_SYMBOLIZER_OPTS=--debug-file-directory=/tmp/dbg \
  $B-stripped test test/aot/no-leak-records-at-exit.test.ts

This branch ends with 31 pass, 0 fail in that first attempt setup of CI, and with $B in place of $B-stripped. main ends with 6 pass, 0 fail.

After #48436 (89452d2)

The branch has main merged in (4429669). On a release ASAN build of that tree the results are the same. This branch ends with 31 pass, 0 fail in both setups. main's file ends with 6 pass, 0 fail. The probe passes with main's file and fails with this one, with the same four records.

The second commit (512f580)

It answers the review of 2b25cf9. Each change has a record or a function of a real program in the tests of written text.

  • testAnywhere now comes from ./harness/default-timeout. the-default-time-limit-never-lowers-what-the-runner-gave.test.ts lists a file that imports test from bun:test and has a time limit. That guard failed at 2b25cf9. It passes now: 11 pass on the release ASAN build.
  • A stack that ends in containers. A debug build of Bun 1.4.3 has such a record in bun run of workers that end with Malloc=1: 724 bytes, 30 functions, all of bmalloc, pas_ and WTF. Its name was /root/.bun/build-cache/.../MakeString.h:156:19 with debug information and WTF::makeAtomString without. It is WTF::makeAtomString for both now.
  • A record without a function (symbolize=0) keeps its text for a name, as before. It matches no record of another process, so the comparison fails and does not pass on nothing.
  • Operators. A small C++ program leaks in 29 functions. They had 17 names: four functions (operator> and operator>=) had const&, eight had app::Version::operator. Now they have 29 names. In a second program a lambda in operator> had the name long.
  • OPENSSL_* and bun_alloc. No record of the five programs has such a frame. A script that calls BoringSSL through bun:ffi leaves eleven records that have them. Release ASAN build: their names were OPENSSL_malloc (7), OPENSSL_realloc (2), OPENSSL_calloc and OPENSSL_memdup. They are now BIO_new, BUF_MEM_new, BN_bin2bn, ASN1_STRING_new, ASN1_STRING_set, BUF_MEM_append, BUF_MEM_reserve and crypto_buffer_new_with_pool.

Over the 266,193 function names of the release ASAN build, against 2b25cf9:

  • 11,830 functions of Rust with generic arguments lose the :: at their end.
  • 18 more get another name: 14 operators with < or >, which had ...::operator, and 4 literal operators of WTF, which had _s, _str, _span and _span8.
  • 724 more are allocators or containers: 666 of bun_alloc, 54 OPENSSL_* and those 4 of WTF. None stops being one.
  • No two names that were apart are one name now.

The 80 records of the five programs (executable and bun run, as built and with Malloc=1) have the same names as at 2b25cf9.

69 changes to single clauses of the parser were tried, one at a time. The 25 tests of written text catch each of them.

On a debug build

  • The five cases that build a program do not pass on a debug build of main, with this change or without it. That was measured at 2b25cf9. main's file: in one run four cases timed out after 270 s, in the next workers that end failed because the build ended with 1. This branch: the same five cases failed, four by the time limit and workers that end by the build.
  • bun-debug build --compile --aot entry.ts child.ts for workers that end ended with a SEGV under AddressSanitizer in 2 of 3 runs. The stack is an add to a WTF::HashTable from JSC::AOT::Typed::LinkFlow::typeOfFunctions (AOTLinkFlow.h:316), on a helper of forEachIndexInParallel. The release ASAN build did not show it in any run of the file.
  • The tests of written text need no compiler. For 512f580 they ran on the release ASAN build and on the released Bun 1.4.3, not on a debug build.

Limits

  • In a debug build BoringSSL's bssl::New<T> is a called function. Six of the eleven BoringSSL records above then have the name bssl::New.
  • The allocation wrappers of other libraries (sqlite3Malloc, ares_malloc), strdup, and Bun's bun_collections have no entry.
  • A function that returns a pointer to a function has the name of its return type. The release ASAN build has one such function.
  • A templated operator< whose first argument begins with ( reads as operator<<. The release ASAN build has none.

From #47443

  • Review there asked for the later rules: containers of Rust, the trait Allocator, -> in a function pointer, a lambda of a constrained function. Each has a record or a method in the tests of written text.
  • Its Notes have the counts over every function name of a release ASAN build and two small programs of Rust that leak on purpose.

Not in this PR

  • The records are leaks that bun run has too. The test accepts a record that bun run also prints.
  • scripts/runner.node.ts and the list of suppressions do not change.

test/aot/no-leak-records-at-exit.test.ts gave every leak record of a
`Malloc=1` run the name `bmalloc::SystemHeap::malloc`, and every record
of Rust the name `alloc`. An executable passes when `bun run` has each
of its names, so a record that only the executable has passed.

A record now has the name of the first called function that is neither
an allocator nor a container. The frames of inlined functions are left
out. `bmalloc::`, `__rustc::`, a class `operator new` and a method of a
trait `Allocator` are allocators. A method of Rust is a container frame
when the language, hashbrown or smallvec has its code. A `requires`
clause, a qualifier and the location are not part of a name.

Tests of written text hold the parser against records that a symbolizer
wrote. They need no compiler and run on every Bun.

This is the change of #47443 on top of main after #47942.
@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The leak-report test parser now selects allocation sites from C++ and Rust stack frames, classifies allocator and container frames, and groups records by the selected site. New tests cover frame parsing and record grouping.

Changes

Leak record attribution

Layer / File(s) Summary
Parse and select allocation sites
test/aot/no-leak-records-at-exit.test.ts
Frame parsing handles C++ and Rust names, removes source locations, classifies allocator and container frames, and selects the allocation site. Tests cover symbolized and unsymbolized stacks.
Group leak records
test/aot/no-leak-records-at-exit.test.ts
Leak records are concatenated under their selected allocation-site key. Tests verify grouping for shared and distinct sites.

Possibly related PRs

  • oven-sh/bun#47443: Changes the same parser to attribute records to the first non-allocator function and adds parser fixtures.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to 512f5

This change only affects how a test names and groups leak records. It has no production impact and no concrete merge-blocking risk was identified.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly summarizes the main change: naming leak records by the function that allocated them.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description explains the problem and fix, and it reports verification results. It covers both required template sections, although it uses different headings.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 98cda0d and 512f580.

Files selected for processing: 1

  • test/aot/no-leak-records-at-exit.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:34 PM PT - Nov 10th, 2026

✅ @robobun, your commit 4429669b5450d703936230b71f169aa592824e59 passed in Build #139828! 🎉


🧪   To try this PR locally:

bunx bun-pr 48461

That installs a local version of the PR into your bun-48461 executable, so you can run:

bun-48461 --bun

@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

This is the change of #47443 on top of main after #47942. A maintainer asked there for a new pull request if the problem still shows, and it does.

Reproduced on a release ASAN build of main (89452d2, after #48436). I gave bun run a one-line program in place of entry.ts and ran the test workers that end. The executable then has four records of its own, and main passes. With this branch that probe fails and lists the four records, and the file ends with 31 pass, 0 fail. The commands are in the Notes of the description.

CI for 512f580 (build 139738) ended with 187 of 188 jobs passed, and the changed file passed on every lane. The red job was :windows: 11 aarch64 - test-bun, by test/aot/parallel/webassembly/shared-memory-atomics/main.ts. This PR does not touch that test. main then got #48436, so the branch now has main merged in (4429669). All review comments are answered and resolved.

@robobun

robobun commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

A note on the red check, as of 2b25cf9. It runs bun bd test test/aot/no-leak-records-at-exit.test.ts on a debug build.

That command does not pass on a debug build of main (98cda0d) without this change either. The five cases that build a program fail there. Some pass the time limit of 270 s. The build of workers that end ends with 1 in 2 of 3 runs: a SEGV under AddressSanitizer in JSC::AOT::Typed::LinkFlow::typeOfFunctions (AOTLinkFlow.h:316). The 20 tests of written text that 2b25cf9 has pass on the debug build.

On a release ASAN build, which the test lanes of CI use, the file ended with 26 pass, 0 fail at 2b25cf9. It ends with 31 pass, 0 fail at 512f580. The change is ready for review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the semantic change in getLeakRecords (same-function records are now concatenated instead of last-write-wins): the two ASAN-gated callers only use the Map keys via plainRecords.has(...) and print the values on failure, so concatenation does not change what any assertion compares. The parser unit tests were not run in this review.

Extended reasoning...

The change is confined to test/aot/no-leak-records-at-exit.test.ts and rewrites the hand-written LSan report parsers (getFunctionName, getCrateOfMethod, isAllocator, frameLocation, getCalledFunctions, getLeakRecords) plus adds fixture-based unit tests for them; it touches no security-sensitive surface. The inline findings already call for a human look at the heuristics, so this note only records the one adjacent concern that reading settled.

- Take `test` for the tests of written text from
  `./harness/default-timeout`. The guard in
  `the-default-time-limit-never-lowers-what-the-runner-gave.test.ts`
  lists a file that imports `test` from `bun:test` and has a time limit.
- A record whose stack ends in allocators and containers has the name
  of the last function on it. Its name came from the text of the
  record, which is different with and without debug information.
- A record that names no function keeps its text for a name, so that
  it is like no other record.
- An operator that is written with `<` or `>` keeps its signs.
  `operator>` had a piece of its arguments for a name.
- An operator for a literal keeps its whole name, so those of WTF are
  containers.
- A function of Rust with generic arguments has no `::` at its end.
- `bun_alloc` and the `OPENSSL_` functions are allocators: BoringSSL
  asks Bun for its memory.
@robobun

robobun commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 512f580. It answers the four review comments on 2b25cf9, and one failure that this PR caused.

  • the-default-time-limit-never-lowers-what-the-runner-gave.test.ts failed because this file imported test from bun:test and has a time limit. The tests of written text now take test from ./harness/default-timeout, as supported.ts does.
  • A stack that ends in containers now gives the name of its last function. The test is a record of a debug build.
  • A function of Rust with generic arguments has no :: at its end.
  • An operator with < or > keeps its signs.
  • OPENSSL_* and bun_alloc are allocators.

On a release ASAN build the file ends with 31 pass, 0 fail, in the first attempt setup and with the whole Bun. The probe with four executable-only records still fails on this branch and passes on main. The 80 records of the five programs have the same names as before this commit. The measurements are in the threads and in the Notes of the description.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the second push addresses everything I raised earlier, each with a fixture test, and I found nothing new.

What was reviewed:

  • Traced getFunctionName by hand through all ten namesOfFunctions fixtures (including the operator<<<int> / operator<<int> backtracking and the cooperator word-boundary case) and getAllocatedBy through both stacks that end in containers — each yields the expected key.
  • Checked the new ./harness/default-timeout import: it exists and wraps bun:test's test, so .each works; .gitattributes pins *.ts to LF, so the template-literal fixtures parse the same on Windows.
  • Not run here: the five ASAN-gated build-and-run cases and the probe described in the PR (no sanitizer build in this review); the pure-text unit tests were checked by reading only.
Extended reasoning...

The change is confined to one test file, test/aot/no-leak-records-at-exit.test.ts, rewriting the LeakSanitizer record parser (operator spellings, Rust crate resolution, allocator/container classification, location stripping, inlined-frame dropping) and adding pure-text unit tests that run on every build. It touches no runtime code and no security-sensitive surface. The four inline findings from my prior review (record-text fallback key, dangling ::, OPENSSL_/bun_alloc shims, operator > depth) are all fixed in commit 512f580 with a regression fixture for each, the bug hunt ran dry, and test/aot is not CODEOWNED, which decided approve.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants