Conversation
…ll pointer The call form of the bun:app Response handed a null out-pointer for the JSX flag to the Rust constructor. The constructor wrote through it for every object body. The constructor and the call form now share one C++ function. It finds out if the body is a JSX element and passes the answer to Rust by value. Response(<jsx />) wraps the element as new Response(<jsx />) does.
|
Status: the fix is pushed. It waits for CI and for #44166. How I reproduced it (every release since 1.3.14, and main a4f1429): // bun nonew.mjs
import { Response } from "bun:app";
console.log(String(Response({ a: 1 })));
|
![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. Since the description leaves the construct-vs-throw semantics of Response(...) without new as an open maintainer decision, a human look is still needed to settle that before merging.
What was reviewed:
- Diffed the old
construct/callpaths against the sharedconstructBakeResponse: the JSX gate is equivalent (isJSXElementalready returns false for non-objects, andisJSXimpliesargumentCount() > 0),argument(1)matches the old undefined default, and the newRETURN_IF_EXCEPTIONafterwrapInnerComponentcloses a gap the oldconstructhad. - Checked the
int*->boolFFI change on both sides ofBakeResponseClass__constructForSSR(C++ extern andjsc_host_abi!export agree;boolis ABI-safe under bothextern "C"andsysv64), and grepped for stale callers ofis_jsx_element/JSC__JSValue__isJSXElement— none remain, including.classes.tsinputs. - The
JSBakeResponsecell left with nullm_ctxon the Rust error path is the pre-existing pattern fromconstruct, not introduced here.
Extended reasoning...
The change touches src/jsc/bindings/JSBakeResponse.cpp, src/runtime/webcore/BakeResponse.rs, and src/jsc/JSValue.rs to fix a null-pointer write in the call form of the bun:app Response by unifying construct and call behind one C++ helper and changing an FFI out-param to a by-value bool, plus five new tests across the two bake test files. It touches no auth, crypto, or injection surface; the security-relevant piece is exception-scope correctness after user-observable $$typeof getter access, which is handled with RETURN_IF_EXCEPTION. The code is small and behavior-preserving on the new path, and the new tests cover both entry points and the streaming-enabled error. Defer rather than approve because the PR itself flags an unresolved user-facing API decision (whether the call form should construct, as implemented, or throw like the global Response), which is a maintainer judgment call rather than a correctness question.
|
No change follows from this review. The PR waits for three things:
|
This branch has not been deployed
2 participants
Stacked on #44166.
A maintainer decision is open: this PR keeps
Response(...)ofbun:appwithoutnewa constructor call. The globalResponsethrows aTypeError. See Notes.Problem
Response(obj)ofbun:appwithoutnewcrashes for every object body:Segmentation fault at address 0x0, debugpanic: null reference produced. Inbun dev, one page that returnsResponse(<jsx />)ends the server.JSBakeResponseConstructor::call(src/jsc/bindings/JSBakeResponse.cpp:228) passesnullptras the out-pointer for the JSX flag.BakeResponseClass__constructForSSR(src/runtime/webcore/BakeResponse.rs:68) writes through it.Fix
constructandcallshare one C++ function,constructBakeResponse. It does the JSX check and passes aboolto Rust.Response(<jsx />)now does whatnew Response(<jsx />)does.JSValue::is_jsx_elementandJSC__JSValue__isJSXElement: no caller is left.test/bake/dev/response-to-bake-response.test.ts(3 new cases) andreact-response.test.ts(2 new cases). All 5 fail on bake: return undefined for an unset AsyncLocalStorage instance #44166.Background
bun:appexports theResponsethat server components get in place of the global one. The parser rewrites the identifier.call.Response(<jsx />)would return a Response that React cannot render.Downsides
Response(obj)andResponse(<jsx />)return a Response where they crashed.Response("x")does not change..textgrows 256 B, becausecallgains the JSX path.new Response("x")ofbun:app: 3363 to 3395 before, 3358 to 3380 after.Notes
The open decision: construct or throw
This PR constructs. Reasons:
JSBakeResponseConstructor::callbuilds an instance, and the doc comment ofresponse_refinsrc/js_parser/p.rsnames the syntaxreturn Response(<jsx />, {...}). Both are from ssg 3 #22138.Response("x"),Response(),Response(null)andResponse(123)ofbun:appreturn a Response on every release build since 1.3.14. No result that works today changes.Reasons to throw
TypeError: Response constructor cannot be invoked without 'new', as the global class and Node do:packages/bun-types/globals.d.tsdeclares only thenewsignature.If the decision is to throw:
callthrows, the 5 new cases assert the error, the comment inp.rsgainsnew, andconstructkeeps the shared function alone.Other designs
calland no other change (2 lines). The crash goes away, butcallignores the flag, soResponse(<jsx />)hastype: nulland no streaming check.Responseclass written as a JS builtin over the generated constructor. It removes the native code that both crashes were in. It also changes dev server behaviour and is a much larger change, so it is not part of a crash fix.How it was found, and who reaches it
bun devthat forgetsnew. The first request of that page ends the dev server for every page.import { Response } from "bun:app". Output ofbun build --server-componentsreaches it from source that only names the globalResponse(cjs and iife today, esm after js_parser: alias the server-components Response import only under hot reloading #44167).Behaviour,
Responseofbun:appResponse("x"),Response()panic: null reference produced)Response({ a: 1 })[object Object]Response(<jsx />, { status: 201 })type()returns the elementResponse(body), the$$typeofgetter ofbodythrowsResponse(<h1 />, { status: 201 })inbun devexport const streaming = truenew Response(<jsx />)givesMeasurements
Release builds (ThinLTO, linux x64) of #44166 (db037a0) and of this branch, from one checkout path.
Instructions per call, from the entry of the host function to its return, callees included. Main thread, counted with a
ptracesingle-step counter, 10 runs of 100 calls, per-run mode, then min / median / max over the runs.valgrind,perfandbloatyare not available in the build container (perf_event_openis not permitted). ASLR cannot be turned off there. That is the spread between runs.bun:app)new Response("x")new Response({ a: 1 })Response("x")Function sizes (
nm -S):BakeResponseClass__constructForSSR408 -> 199 B.JSBakeResponseConstructor::construct1311 -> 904 B.::call252 -> 494 B.wrapInnerComponentwas inline inconstructand is now one copy of 621 B.isJSXElement310 -> 487 B, andreactLegacyElementSymbol(203 B) is inline in it now.Binary (
size -A,ls -l):.text58,143,477 -> 58,143,733 B.bun-profile169,905,600 -> 169,903,232 B. Strippedbun80,827,976 B in both.Startup: both
Zig::GlobalObjectconstructors keep their size (708 B and 717 B).Codegen: cppbind rows 173 -> 173. JS-to-native host functions 368 -> 368.
Cells per JSX construction in the dev server (
heapStats, 2000 kept alive): 16.01 fornewon bake: return undefined for an unset AsyncLocalStorage instance #44166, 16.01 fornewon this PR, 16.01 for the call form on this PR.Suites
BUN_JSC_validateExceptionChecks=1and LeakSanitizer as the ASAN lane sets them:response-to-bake-response.test.ts11 of 11.react-response.test.ts13 of 13 (exception checks only, the file is inno-validate-leaksan.txt).production.test.ts13 of 13,ssg-pages-router.test.ts9 of 9,request-cookies.test.ts2 of 2.bun run rust:check-all: 12 of 12 targets.BakeResponseClass__constructForSSRkeeps its calling convention. Only the type of one parameter changes, fromint*tobool.Other open PRs on this code
getAsyncLocalStorage(). This PR does not touchBakeAdditionsToGlobalObject.horVirtualMachine.rs.response-to-bake-response.test.ts. The 3 new cases are at the end of thedescribeblock of bake: return undefined for an unset AsyncLocalStorage instance #44166, below the lines that js_parser: alias the server-components Response import only under hot reloading #44167 changes.BakeResponseClass__constructForSSRto&mut c_int. The PR that lands second must take thebool.Defects that this PR does not change
Each one gives the same result on #44166 and on this branch. None is a crash.
bun dev,Response.redirect(url, 301)(also 307, 308) answers 200 with the target page and noLocation.hmr-runtime-server.tstells a redirect from a render withstatus !== 302.instanceofthebun:appclass is false for afetch()result, forResponse.json(), forclone()and for a global instance.Bun.inspect()of abun:appResponse prints<null />.Response.redirect(),Response.render()and a JSX body then throwTypeError: store value must have a "streaming" field. The call form with a JSX body now does the same.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file