Repository navigation
Conversation
…a large environment On the Linux main thread, WTF measured RLIMIT_STACK from below the argument and environment strings. The kernel measures it from the end of the stack mapping, above them. The bound was too low by their size, and with enough of them no recursion check tripped before the kernel's limit. Pin the preview build of oven-sh/WebKit#556, which measures from the end of the mapping, and add the test.
|
Reproduced on a release build from before #47035 (
The diff is green. In build 133801 on |
|
Updated 11:51 AM PT - Oct 27th, 2026
✅ @robobun, your commit 4fd82e216f0dac7509cb035c20bf9e956399361d passed in 🧪 To try this PR locally: bunx bun-pr 44302That installs a local version of the PR into your bun-44302 --bun |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
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. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… skip under a low hard limit The [define] cases compare stdout, stderr, the exit code and the signal in one object. stderr must match the whole error report, with any column. The JS recursion case runs under a 5 MB stack limit, was 4 MB. exec allows a quarter of the limit for the arguments and the environment, so the inherited environment can have 550 KB, was 350 KB. A build without the WebKit change still gets SIGSEGV under 5 MB. The cases are skipped when the hard stack limit is below 8 MB, because the soft limit cannot go above it.
There was a problem hiding this comment.
Code review found no issues
No high-confidence issues detected in this change.
Still open from earlier reviews (1):
- 🔴
scripts/build/deps/webkit.ts:6—Every build breaks the moment oven-sh/WebKit#556 merges or closes, because WEBKIT_VERSION now names a preview release G…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
robobun
changed the title
|
@dylan-conway the pin here cannot follow oven-sh/WebKit#755 yet.
I made this pull request a draft and rewrote its description. When #755 is merged, or has a preview build on the current WebKit |
…k-bounds-large-environment # Conflicts: # scripts/build/deps/webkit.ts
main pins a WebKit with endOfMainThreadStack() since #47035, so this branch no longer changes the pin. The case that starts bun through glibc's loader still gets SIGSEGV. It is a todo until oven-sh/WebKit#755 is in the pin.
robobun
changed the title
There was a problem hiding this comment.
I reviewed the reduced, test-only version of this PR and found no bugs; since the WebKit pin change is gone and the earlier inline points (hard-limit skip, exact stderr matching) are now handled in the file, a human look is still worthwhile mainly to confirm the alpine lanes agree with the loader case, which the description says was not run on musl.
What was reviewed:
- The four active cases and the
todoIf(isGlibc)loader case in test/js/bun/jsc/stack-bounds-large-environment.test.ts against harness conventions (bunEnv spread, tempDir withusing, concurrent pipe draining,signalCode: null, Buffer.alloc padding). - The /proc/self/limits hard-limit probe (third column is the hard limit) and the ELF64 PT_INTERP parse; both only run on Linux and both Linux targets are 64-bit little-endian.
- The expected "JSON document is too deeply nested" text matches src/parsers/json.rs; the loader test's todo will become a failure once a WebKit with oven-sh/WebKit#755 is pinned, which is the stated intent.
Extended reasoning...
The diff adds one 171-line Linux-only test file and touches no source, build or dependency files; the WebKit pin in scripts/build/deps/webkit.ts is unchanged from the base at 191c6a41380b. It touches no security-sensitive surface. It is self-contained and cannot affect the product, which argues for approval, but the loader case depends on musl vs glibc ld.so behaviour that the author states was only checked via CI, and the todo-on-glibc pattern is an intentional time bomb that a maintainer should agree with before merge, so a human confirmation is still worthwhile.
|
Two facts for the review above.
|
Problem
Segmentation faulton deeply nested input: a[define]value,Bun.JSONC.parse,new RegExp. It needed a stack limit and about 128 KB of arguments and environment.StackBounds.cpp) measured the main thread's stack bound from below those strings. The kernel counts them againstRLIMIT_STACK.endOfMainThreadStack(), [JSC] Ahead-of-time compilation WebKit#757). No test in bun covers it.Fix
test/js/bun/jsc/stack-bounds-large-environment.test.ts. The branch no longer changes the WebKit pin.ulimit -S -s: bun's parsers and transpiler, a 40,000-level[define]value, a nestedRegExp, JS recursion.test.todoIf(isGlibc): glibc's loader movesAT_EXECFN, and the child still gets SIGSEGV. [WTF] Linux: find the end of the main thread's stack when AT_EXECFN points to argv[0] WebKit#755 corrects that.a0cc096ce. The release build367d939d9, from before Upgrade WebKit to 982277c9dd3e #47035, fails 4 of 4 with SIGSEGV. CI passed the file on seven Linux lanes, with the loader case on alpine.Background
bun_core::StackCheckcompares the stack pointer with a bound and keeps 128 KB free. JavaScriptCore reads the same bound, fromWTF::StackBounds.execve()puts the argument and environment strings at the end of the main thread's stack mapping.AT_EXECFNpoints to the path of the executable, the last string.test.todoIfcase. Considered leaving the loader case out: the todo keeps the failure and its correction in the file.Notes
Repro, x64,
ulimit -s 8192:367d939d9, from before Upgrade WebKit to 982277c9dd3e #47035:Segmentation fault, exit 139. WithoutBIGVARit printserror: JSON document is too deeply nestedand exits 1.a0cc096ce, which is Upgrade WebKit to 982277c9dd3e #47035:error: JSON document is too deeply nested, exit 1.Before the correction, a default batch of GNU
xargs(131,072 bytes of arguments) was enough to reach the crash. Withulimit -s unlimitedthe kernel has no limit to reach, and nothing crashes. No user reported it. A review of #41519 found it.How the correction reached
main.endOfMainThreadStack()toSource/WTF/wtf/StackBounds.cpp.680ef47cec, the parent of that commit, to982277c9dd3e.AT_EXECFNpoints toargv[0]: glibc'sld.sorun as a command, and PRoot.The four cases depend on the bound with the current code. The canary, started through glibc's loader, gets a bound that is too low by the size of the environment. All four cases then fail with SIGSEGV.
The loader case. On glibc it fails on the debug build and on the canary with exit 139 (SIGSEGV), so it is a todo there. musl's loader only reads
AT_EXECFN(ldso/dynlink.c), so the case is a normal test on the alpine lanes. When bun pins a WebKit with oven-sh/WebKit#755,testWithLoaderbecomestest.skipIf(!loader).bun test --todoreports a todo case that passes as a failure.The test runs each case under
ulimit -S -s, because CI runs with an unlimited stack. The JS recursion case runs under 5 MB: a build without the correction gets SIGSEGV up to 5.5 MB, and from 6 MB JavaScriptCore's own 5 MB cap stops the recursion first. exec allows a quarter of the limit for the arguments and the environment, so under 5 MB the inherited environment can have about 550 KB. The RegExp case uses fewer groups and a 4 MB limit in a debug build, where JavaScriptCore takes 0.2 ms for each group. The cases are skipped when the hard stack limit is below 8 MB.CI, build 133801 on
4fd82e216f. The file passed on debian 13 (x64, aarch64, x64-asan), ubuntu 25.04 (x64, aarch64) and alpine 3.23 (x64, aarch64). Every one of these lanes runs with an unlimited stack limit, so no case was skipped. On alpine the loader case is not a todo, so it ran and passed there. The log has one line for each file, not for each case. I did not run a musl build locally.Earlier CI. With the pin on the preview build of oven-sh/WebKit#556 (2026-09-30), the five cases passed on debian and ubuntu (x64 and arm64), on alpine (x64 and arm64) and on the x64 ASAN lane.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/jsc/stack-bounds-large-environment.test.ts