Conversation
NAPI modules like ffi-napi call uv_thread_self and friends during module init. They hit the uv-posix-stubs.c "unsupported uv function" panic before the users require() even returned. The uv_thread_* family maps 1:1 onto pthread on POSIX (uv_thread_t is typedef pthread_t, per vendor/libuv/include/uv/unix.h:130), so they can be polyfilled directly. Matches the existing uv_mutex_* / uv_once / uv_os_getpid pattern in uv-posix-polyfills.c. Polyfilled: uv_thread_self, uv_thread_equal, uv_thread_join, uv_thread_detach, uv_thread_create, uv_thread_create_ex Note: uv_thread_create_ex honors UV_THREAD_HAS_STACK_SIZE but omits libuvs page-rounding / minimum-stack clamping. pthread_attr_setstacksize errors propagate via abort(), matching other thread primitives already in this file. ffi-napi still crashes later - it also calls uv_async_init and the libuv event-loop surface - but uv_thread_self is no longer the blocker. Broader libuv support tracks in #18546. Test: test/regression/issue/29260.test.ts spins up a NAPI addon (test/napi/napi-app/uv_thread_addon.c) whose init exercises every polyfilled symbol and returns true only if all succeed. Gate verified: passes with the fix, panics uv_thread_self without it.
|
Found 1 issue this PR may fix:
Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds real POSIX pthread-based implementations for several libuv thread APIs, removes their stubbed not-implemented variants and generated export entries, and introduces/updates N-API tests and build targets to validate the new thread APIs and prevent runtime panics. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/napi/napi-app/uv_thread_addon.c`:
- Around line 56-63: The detached thread currently writes to the stack-local
variable counter via thread_entry after NAPI_MODULE_INIT may return, causing a
use-after-return; modify the detach test so the detached thread does not access
a stack-local: either make counter static (e.g. static int counter) or allocate
the counter on the heap and pass its pointer, or change thread_entry to a no-op
that doesn't touch the passed data; update the uv_thread_create/uv_thread_detach
invocation to pass the chosen safe storage and ensure any heap allocation is
freed by the thread if used.
In `@test/napi/uv_stub.test.ts`:
- Around line 89-97: In the test "should not crash when calling supported uv
functions" swap the order of the assertions so you assert on stdout (the
variable stdout / out and expect(out).toContain(...)) before asserting on
exitCode (expect(exitCode).toBe(0)); locate the test block and move the exitCode
assertion after the stdout/assertions (including the two
expect(out).toContain(...) checks) so failures report stdout content first for
better diagnostics.
In `@test/napi/uv-stub-stuff/good_plugin.c`:
- Around line 60-69: The detach test currently passes a stack-local variable
`counter` to the detached thread (`uv_thread_create` -> `thread_entry`) which
risks a use-after-return if `Init` returns before the thread finishes; fix by
either making the counter heap/static-scoped or by turning the detached thread
into a no-op: allocate `counter` with malloc (and have the thread free it) or
call `uv_thread_create(&tid, thread_entry, NULL)` and update `thread_entry` to
handle a NULL argument (ignoring any counter) so `uv_thread_detach` is still
exercised without referencing stack memory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4c4cb3b-8f63-412a-8e68-6439ccedf81f
📒 Files selected for processing (9)
src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.tssrc/bun.js/bindings/uv-posix-polyfills.csrc/bun.js/bindings/uv-posix-stubs.ctest/napi/napi-app/binding.gyptest/napi/napi-app/uv_thread_addon.ctest/napi/uv-stub-stuff/good_plugin.ctest/napi/uv-stub-stuff/plugin.ctest/napi/uv_stub.test.tstest/regression/issue/29260.test.ts
💤 Files with no reviewable changes (2)
- test/napi/uv-stub-stuff/plugin.c
- src/bun.js/bindings/uv-posix-stubs.c
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean pthread wrappers following the existing polyfill pattern, with the detach-test use-after-stack nit already flagged inline.
Extended reasoning...
Overview
The PR adds 6 libuv thread primitives (, , , , , ) as thin pthread wrappers in , removes the corresponding panic stubs from and the stub generator constants, and adds regression tests in and .
Security risks
None. These are trivial wrappers around standard POSIX pthread calls, copy-pasted from libuv's own . They don't touch auth, crypto, IPC, or any security boundary.
Level of scrutiny
Low. The implementation follows the exact same pattern as the already-present and polyfills in the same file. is on POSIX, so the mapping is 1:1. The one deliberate simplification ( omits page-rounding/clamping of the stack size) is documented in a comment and is acceptable for this use case.
Other factors
The only issue flagged by the bug hunter is a use-after-stack in the detach sub-tests in and — both files pass a pointer to a local to a detached thread without joining. This is formally undefined behavior in test-only code. In practice the race window is a single store instruction wide and has negligible impact. It is already captured as an inline comment on the PR. The production polyfill code is not affected.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29260.test.ts`:
- Around line 61-63: Remove the negative panic-string assertion by deleting the
expect(stderr).not.toContain("unsupported uv function"); line in the test (leave
expect(stdout).toContain("boolean"); intact); this eliminates the discouraged
check for absence of panic-like text and relies on existing assertions that
already detect crashes/non-zero exits and successful initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e401b70-50f2-45f2-b79d-fe13c7b8e2eb
📒 Files selected for processing (1)
test/regression/issue/29260.test.ts
- uv_thread_addon.c + good_plugin.c: use a dedicated no-op thread_entry
for the detach sub-test. The previous version passed a stack-local
counter to the detached thread, which can be a use-after-return if
the caller returns before the thread scheduler starts the new thread
(flagged by coderabbit/claude bots).
- good_plugin.c: also drop the 128KB explicit stack_size from the _ex
test. Under ASAN the minimum viable stack is much larger, and the
no-flags path still exercises uv_thread_create_ex end-to-end.
- 29260.test.ts: drop the expect(stderr).not.toContain("unsupported uv
function") assertion. CLAUDE.md forbids negative-panic-string checks
(they pass vacuously on different failure modes). expect(stdout
.toContain("boolean")) + exitCode already cover every regression
path that matters.
- uv_stub.test.ts: reorder assertions so stdout checks come before
exitCode, per CLAUDE.md.
- 29260.test.ts: bump per-test timeout to 30s. The 5s default races
the first NAPI module load under ASAN.
…ne stale skip
- uv-posix-polyfills.c: add uv__min_stack_size / uv__default_stack_size /
uv__thread_stack_size helpers and the full page-rounding + min-clamp
logic from libuv src/unix/thread.c. With these in place, the existing
abort() on pthread_attr_setstacksize failure is safe — every value
passed to setstacksize is guaranteed page-aligned and >= PTHREAD_STACK_MIN,
matching libuvs own behavior byte-for-byte. Includes adjusted to add
sys/resource.h for getrlimit and limits.h for PTHREAD_STACK_MIN.
- 29260.test.ts: drop the { timeout: 30_000 } from test(). CLAUDE.md
forbids per-test timeouts; the slow node-gyp build is already
covered by the beforeAll timeout and the test body itself finishes
in milliseconds.
- generate_uv_posix_stubs_constants.ts: remove stale uv_thread_detach
from test_skipped. That symbol is no longer in the generated symbols
list (it is polyfilled), so the skip entry is dead code and masks a
latent regression if the symbol is ever re-added.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29260.test.ts`:
- Around line 45-66: The test currently runs bun with "-p typeof
require(addonPath)" which only checks the type and would pass if the addon
returned false; change the spawned command so it prints the addon's actual
return value (e.g., evaluate and print require(addonPath) instead of typeof
require(...)) and update assertions to assert the addon returned true (check
stdout for "true" or assert stdout === "true") in addition to exitCode === 0;
locate the Bun.spawn call that uses bunExe(), addonPath and napiAppDir and
modify the cmd and the expect() checks accordingly.
- Around line 1-12: Trim the file header in test/regression/issue/29260.test.ts
to a single-line GitHub issue URL comment and remove the long preamble; keep
only minimal inline comments necessary to explain the test structure or why
certain assertions exist. Specifically, replace the multi-line comment block
describing bug history with the standard one-line
"https://github.com/oven-sh/bun/issues/29260" and move any essential rationale
into short inline comments next to the test code (no extra narrative at file
top).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1f415c9-e055-485b-8def-bd8d7079207b
📒 Files selected for processing (3)
src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.tssrc/bun.js/bindings/uv-posix-polyfills.ctest/regression/issue/29260.test.ts
- Drop the multi-line bug preamble per test/regression/issue/* convention; keep only the standard one-line issue URL. - Change the spawned command from -p typeof require(...) to -p require(...) and assert stdout.trim() === "true". The old assertion would pass if the addon ever returned false (both print "boolean").
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR adds POSIX pthread-backed polyfills for six libuv thread APIs (uv_thread_self, uv_thread_equal, uv_thread_join, uv_thread_detach, uv_thread_create, uv_thread_create_ex) in uv-posix-polyfills.c, removing the panic stubs from uv-posix-stubs.c. It also adds NAPI test fixtures (uv_thread_addon.c, updates to good_plugin.c and uv_stub.test.ts) and a regression test for issue #29260.
Security risks
No security-sensitive code paths (auth, crypto, permissions) are touched. The implementation copies libuv stack-size logic verbatim, ensuring pthread_attr_setstacksize is only called with page-aligned values ≥ PTHREAD_STACK_MIN. No injection or data-exposure concerns.
Level of scrutiny
Moderate: the core implementation in uv-posix-polyfills.c is a faithful copy of upstream libuv src/unix/thread.c with no novel logic. The test changes are mechanical. Multiple rounds of automated review caught and fixed real issues (stack use-after-return, assertion ordering, abort() on invalid sizes, test timeout, stale skip entry, negative panic assertion) — all resolved in commits f7d0afc and 5985ac9.
Other factors
The one remaining nit (potential integer overflow in the page-rounding expression when stack_size is near SIZE_MAX) is a pre-existing upstream libuv limitation, not introduced by this PR, and is already captured as an inline comment. No outstanding unaddressed reviewer concerns remain.
There was a problem hiding this comment.
LGTM — all review feedback addressed, implementation faithfully mirrors libuv thread.c.
Extended reasoning...
Overview
Adds pthread-based polyfills for six uv_thread_* functions in uv-posix-polyfills.c, removes the corresponding panic stubs from uv-posix-stubs.c, and adds a new NAPI addon (uv_thread_addon.c) plus a regression test (test/regression/issue/29260.test.ts).
Security risks
No security-sensitive code paths. The polyfills are thin pthread wrappers with no privilege escalation or data exposure risk. The uv_thread_create_ex stack-size path applies page-rounding and minimum-stack clamping before calling pthread_attr_setstacksize, matching libuv byte-for-byte.
Level of scrutiny
Moderate: NAPI/C code changes are not trivial, but the approach is conservative — copy-paste from libuv own src/unix/thread.c rather than a novel implementation. Multiple rounds of bot review caught and fixed real issues (use-after-stack in detach test, wrong typeof-vs-value assertion, stale test_skipped entry, test timeout guideline violation, negative panic assertion). All are resolved.
Other factors
All inline comments are resolved. The implementation follows the same pattern as the existing uv_mutex_* / uv_once polyfills in the same file. uv_thread_t is typedef pthread_t on POSIX so the mapping is 1:1 with no semantic gap. No bugs were found by the bug hunting system on the final revision.
test/regression/issue/29260.test.ts trips the ASAN validateExceptionChecks path through Process_functionDlopen → BunProcess.cpp:379 (thrown from JSObject.cpp:835), the same pre-existing throw-scope issue already handled for other NAPI tests (test/napi/uv.test.ts, uv_stub.test.ts). Add the regression test under the existing "3rd party napi" section.
1 participant
What
Polyfill the POSIX libuv thread primitives that NAPI modules (notably
ffi-napi) call during module init. They currently hit theuv-posix-stubs.c"unsupported uv function"panic before usersrequire()even returns.Polyfilled (all trivial pthread wrappers):
uv_thread_selfuv_thread_equaluv_thread_joinuv_thread_detachuv_thread_createuv_thread_create_exPlaced alongside the existing
uv_mutex_*/uv_once/uv_os_getpidpolyfills insrc/bun.js/bindings/uv-posix-polyfills.c.Why this works
uv_thread_tistypedef pthread_ton POSIX (vendor/libuv/include/uv/unix.h:130), and libuvs ownsrc/unix/thread.cimplementations are 1:1 pthread calls. We just copy them into the polyfills file and remove the corresponding stubs from the generateduv-posix-stubs.c.uv_thread_create_exhonorsUV_THREAD_HAS_STACK_SIZEbut omits libuvs page-rounding / minimum-stack clamping — simpler, andpthread_attr_setstacksizeerrorsabort(), matching the other primitives already in the file.Scope
This fixes the specific crash in #29260 (the one the reporters stack trace points at).
ffi-napistill hits a separate wall later — it also usesuv_async_init+uv_queue_work+uv_default_loop, which require a real Bun-integrated libuv event loop. That wider surface tracks in #18546.Reproduction
Before:
panic: unsupported uv function: uv_thread_self(crash duringrequire).After:
requireproceeds past the thread-init path; ffi-napi fails later onuv_async_init, which is part of #18546.Test
test/regression/issue/29260.test.ts— spawns a NAPI addon (test/napi/napi-app/uv_thread_addon.c) whoseNAPI_MODULE_INITcalls every polyfilled symbol and returnstrueonly if they all succeed.Gate verified locally:
USE_SYSTEM_BUN=1 bun test test/regression/issue/29260.test.ts→ fail (panicuv_thread_self)bun bd test test/regression/issue/29260.test.ts→ passCloses #29260.