ffi: reject non-void return types for threadsafe JSCallback - #31779
ffi: reject non-void return types for threadsafe JSCallback#31779EffortlessSteven wants to merge 1 commit into
Conversation
A threadsafe JSCallback posts work to the JS event loop and cannot
synchronously return a value to native code, so its return type must be
void. The validation guard checked `function.threadsafe` before the
parsed `threadsafe` flag had been assigned to it, so it never fired: a
non-void threadsafe callback was accepted and wired to the void
trampoline, returning garbage to the native caller.
Check the parsed local flag so the existing "Threadsafe functions must
return void" validation runs. Also surface the error the way dlopen()
does: the JSCallback constructor destructures `{ ctx, ptr }` from the
native return, so an error returned as a value was silently dropped and
left a callback with `ptr === undefined`; throw it instead.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
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 (2)
WalkthroughThis PR improves FFI callback construction error handling. The implementation now throws validation errors as JavaScript exceptions rather than returning them, and fixes the threadsafe callback validation to check the correct flag state. Tests verify both the threadsafe return-type constraint and unknown return-type error handling are properly enforced at construction time. ChangesFFI Callback Error Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
I opened #32782 without having seen this PR; my duplicate search was scoped too narrowly and missed it. You found and fixed the dead The two are not identical: #32782 puts the guard in I am not closing either PR; that is a maintainer's call. If they prefer this one, I will close #32782 and send a separate PR for the |
|
Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @EffortlessSteven for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
3 participants
What this does
Safe JS can construct a thread-safe
JSCallbackwith a non-void return type. A thread-safe callback is delivered asynchronously and cannot return a value to native code, so its caller reads garbage. Bun accepts it instead of rejecting it.Fix: reject the non-void case (check the parsed
threadsafeflag), and throw the error likedlopen()instead of returning a value the constructor silently dropped.Verification
Red on
main, green after.JSCallbackThreadsafe functions must return voidJSCallbackoptionsptr === undefinedfmt,clippyReview map
ffi_body.rs(guard): check the parsed localthreadsafe, not the stalefunction.threadsafeffi_body.rs(callback): throw the construction error, not return itffi.test.js: non-void threadsafe rejected; invalid options throw