Repository navigation
orderfile: stop a traced process's children from truncating its trace - #33347
Conversation
pagetrace.c's constructor opened BUN_PAGETRACE_OUT with O_CREAT|O_TRUNC and ftruncated it before reading /proc/self/maps to find out whether this process is the binary under trace. LD_PRELOAD and both BUN_PAGETRACE_* variables are inherited across exec, so any dynamically linked child a workload spawns — a lifecycle script, /bin/sh, another bun — re-ran the constructor, zeroed the MAP_SHARED trace its parent was still writing into, and only then bailed. The pages lost are the earliest-touched ones, which is to say the hottest. Find the regions first and leave the file alone when there are none, and drop LD_PRELOAD and the two BUN_PAGETRACE_* variables from the environment once loaded, so a child that does map the same binary never arms itself either. ptyrun already hands the preload down explicitly rather than relying on inheritance, so nothing depended on it. Nothing in today's workloads spawns, and the order file comes out the same (37.9k functions, same per-workload page counts), but `bun install` is one package.json away from running a lifecycle script. The new test traces a fixture that reads 32 pages, execs a child, then reads one more: before this it recorded 1 page, now it records all 33. Reported by claude[bot] on #33302.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe pagetrace.c constructor now reads and unsets environment variables (LD_PRELOAD, BUN_PAGETRACE_BIN/OUT) before scanning /proc/self/maps, deferring output-file creation/mmap until after region discovery. ptyrun.c's comment is updated accordingly. A new test suite and C fixture validate page-tracing behavior across fork/exec. ChangesPagetrace init reordering and tests
Sequence Diagram(s)sequenceDiagram
participant Test as linker-order.test.ts
participant Fixture as pagetrace-fixture
participant Tracer as pagetrace.c (LD_PRELOAD)
participant Child
Test->>Fixture: spawn with LD_PRELOAD=tracer, BUN_PAGETRACE_BIN/OUT
Fixture->>Fixture: touch .rodata pages (STRIDE reads)
Fixture->>Child: fork + execl(child)
Child-->>Fixture: exit
Fixture->>Fixture: final touched-page read, print sum
Test->>Test: read trace output, parse header, assert results
Estimated code review effort: Medium Suggested labels: testing, linux, orderfile Suggested reviewers: none identified from provided context Poem 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — focused build-tooling fix with a solid regression test.
Extended reasoning...
Overview
This PR reorders scripts/orderfile/pagetrace.c's constructor so it checks /proc/self/maps for the traced binary's regions before opening/truncating the output file, and additionally unsetenvs LD_PRELOAD + the two BUN_PAGETRACE_* vars once loaded so exec'd children never re-arm the tracer. The env values are correctly copied to stack buffers before unsetenv (which may invalidate getenv returns). ptyrun.c gets a comment-only update. A new C fixture and test in test/js/bun/perf/linker-order.test.ts verify a child exec no longer wipes the parent's trace.
Security risks
None. This is a Linux-only, build-time LD_PRELOAD shim used exclusively by scripts/orderfile/generate.ts during release builds to produce the linker symbol-ordering file. It is not shipped in the bun binary and has no user-facing surface.
Level of scrutiny
Low-to-medium. The changed code is developer tooling under scripts/, not runtime code — worst case is a suboptimal .text layout, which the existing verifyOrderFileApplied() CI check would still catch. The core change is a mechanical reordering of two existing blocks (region discovery ↔ file creation) plus three unsetenv calls, and the PR description confirms the generated order file is byte-identical for all current workloads.
Other factors
- Follow-up to a specific review comment on #33302; scope is exactly what was asked for.
- The regression test is well-constructed: compiles the real tracer, uses a fixture that touches 32 distinct 64 KB-strided
.rodatapages, fork+execs a dynamically linked child, then touches one more — asserting ≥33 recorded faults. PR shows it fails (count=1) before the fix and passes after. Skipped on non-Linux / no compiler, usestempDir/bunEnv/await using, drains pipes concurrently. - The
compile()helper was hoisted unchanged from the existingpty runnerdescribe block to share it — no behavior change there. - Minor: the test asserts
stderr: ""on the fixture, which CLAUDE.md discourages for bun-debug spawns, but this is a plaincc -O1C binary with no sanitizers, so it's fine here.
Follow-up to #33302, per this review comment.
The bug
pagetrace.c's constructor openedBUN_PAGETRACE_OUTwithO_CREAT|O_TRUNCandftruncated it before reading/proc/self/mapsto find out whether this process is even the binary under trace:LD_PRELOADand bothBUN_PAGETRACE_*variables are inherited acrossexec, so any dynamically linked child a workload spawns — a lifecycle script,/bin/sh, another bun — re-ran the constructor, zeroed theMAP_SHAREDtrace its parent was still writing into, and only then bailed. The pages lost are the earliest-touched ones, which is to say the hottest.The fix
Find the regions first and leave the file alone when there are none. Then drop
LD_PRELOADand the twoBUN_PAGETRACE_*variables from the environment once loaded, so a child that does map the same binary (another bun) never arms itself either — the region check alone can't catch that one.ptyrunalready hands the preload down explicitly viaPTYRUN_PRELOADrather than relying on inheritance, so nothing depended on it; its comment is updated to say so.Impact today
None, which is why it was still there. No workload currently spawns, and the order file comes out byte-for-byte the same work:
But
bun installis onepackage.jsonaway from running a lifecycle script, and the failure is silent: a truncated trace still produces an order file, just one missing the hot core.Test
test/js/bun/perf/pagetrace-fixture.creads one byte from each of 32 pages of its own.rodata, execs a dynamically linked child, then reads one more. Under the tracer each read is a recorded fault, so the count is the signal:The stride is 64 KB so the page count is the same on kernels with 16 KB or 64 KB pages. Skipped where there's no C compiler, alongside the existing
ptyruntest.