Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export const symbols = [
"uv_handle_size",
"uv_handle_type_name",
"uv_has_ref",
// Defined in uv-posix-polyfills.cpp
// Defined in uv-posix-polyfills.c
// "uv_hrtime",
"uv_idle_init",
"uv_idle_start",
Expand Down Expand Up @@ -162,15 +162,15 @@ export const symbols = [
"uv_loop_size",
"uv_metrics_idle_time",
"uv_metrics_info",
// Defined in uv-posix-polyfills.cpp
// Defined in uv-posix-polyfills.c
// "uv_mutex_destroy",
// "uv_mutex_init",
// "uv_mutex_init_recursive",
// "uv_mutex_lock",
// "uv_mutex_trylock",
// "uv_mutex_unlock",
"uv_now",
// Defined in uv-posix-polyfills.cpp
// Defined in uv-posix-polyfills.c
// "uv_once",
"uv_open_osfhandle",
"uv_os_environ",
Expand All @@ -182,9 +182,9 @@ export const symbols = [
"uv_os_get_passwd2",
"uv_os_getenv",
"uv_os_gethostname",
// Defined in uv-posix-polyfills.cpp
// Defined in uv-posix-polyfills.c
// "uv_os_getpid",
// Defined in uv-posix-polyfills.cpp
// Defined in uv-posix-polyfills.c
// "uv_os_getppid",
"uv_os_getpriority",
"uv_os_homedir",
Expand Down Expand Up @@ -280,7 +280,8 @@ export const symbols = [
"uv_thread_getname",
"uv_thread_getpriority",
"uv_thread_join",
"uv_thread_self",
// Defined in uv-posix-polyfills.c
// "uv_thread_self",
"uv_thread_setaffinity",
"uv_thread_setname",
"uv_thread_setpriority",
Expand Down
7 changes: 7 additions & 0 deletions src/bun.js/bindings/uv-posix-polyfills.c
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ UV_EXTERN void uv_once(uv_once_t* guard, void (*callback)(void))
abort();
}

// Copy-pasted from libuv (src/unix/thread.c).
// uv_thread_t is pthread_t on POSIX (see uv/unix.h).
UV_EXTERN uv_thread_t uv_thread_self(void)
{
return pthread_self();
}

UV_EXTERN uint64_t uv_hrtime(void)
{
return uv__hrtime(UV_CLOCK_PRECISE);
Expand Down
6 changes: 0 additions & 6 deletions src/bun.js/bindings/uv-posix-stubs.c
Original file line number Diff line number Diff line change
Expand Up @@ -1745,12 +1745,6 @@ UV_EXTERN int uv_thread_join(uv_thread_t* tid)
__builtin_unreachable();
}

UV_EXTERN uv_thread_t uv_thread_self(void)
{
__bun_throw_not_implemented("uv_thread_self");
__builtin_unreachable();
}

UV_EXTERN int uv_thread_setaffinity(uv_thread_t* tid,
char* cpumask,
char* oldmask,
Expand Down
6 changes: 0 additions & 6 deletions test/napi/uv-stub-stuff/plugin.c
Original file line number Diff line number Diff line change
Expand Up @@ -2089,12 +2089,6 @@ napi_value call_uv_func(napi_env env, napi_callback_info info) {
return NULL;
}

if (strcmp(buffer, "uv_thread_self") == 0) {

uv_thread_self();
return NULL;
}

if (strcmp(buffer, "uv_thread_setaffinity") == 0) {
uv_thread_t *arg0 = {0};
char *arg1 = {0};
Expand Down
20 changes: 20 additions & 0 deletions test/napi/uv-stub-stuff/uv_impl.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <node_api.h>

#include <pthread.h>
#include <signal.h>
#include <stdio.h>
#include <string.h>
Expand Down Expand Up @@ -102,6 +103,22 @@
return ret;
}

// Test uv_thread_self — regression test for #29223.
// ffi-napi calls uv_thread_self during NAPI module init; Bun used to panic
// because uv_thread_self was a stubbed libuv symbol on POSIX.
static napi_value test_thread_self(napi_env env, napi_callback_info info) {
// Call uv_thread_self twice — both from the current (main) thread. They
// must refer to the same thread per pthread_equal.
uv_thread_t a = uv_thread_self();
uv_thread_t b = uv_thread_self();

napi_value ret;
// pthread_equal returns non-zero for equal threads. Return a boolean so
// the JS side can check it without caring about uv_thread_t layout.
napi_get_boolean(env, pthread_equal(a, b) != 0, &ret);
return ret;
}

Check warning on line 120 in test/napi/uv-stub-stuff/uv_impl.c

View check run for this annotation

Claude / Claude Code Review

test_thread_self only checks self-consistency, not correctness vs pthread_self()

The `test_thread_self` function in `test/napi/uv-stub-stuff/uv_impl.c` (line 118) checks `pthread_equal(a, b) \!= 0` — verifying only that two consecutive `uv_thread_self()` calls agree with each other — but never checks `pthread_equal(a, pthread_self())`. A bogus implementation returning any constant `pthread_t` value would pass. Fix: add `&& pthread_equal(a, pthread_self()) \!= 0` to the condition on line 118 to match the fuller check already present in the companion addon in `29223.test.ts`.
Comment thread
robobun marked this conversation as resolved.

// Test uv_hrtime
static napi_value test_hrtime(napi_env env, napi_callback_info info) {
uint64_t time1 = uv_hrtime();
Expand Down Expand Up @@ -150,6 +167,9 @@
napi_create_function(env, NULL, 0, test_uv_once, NULL, &fn);
napi_set_named_property(env, exports, "testUvOnce", fn);

napi_create_function(env, NULL, 0, test_thread_self, NULL, &fn);
napi_set_named_property(env, exports, "testThreadSelf", fn);

napi_create_function(env, NULL, 0, test_hrtime, NULL, &fn);
napi_set_named_property(env, exports, "testHrtime", fn);

Expand Down
6 changes: 6 additions & 0 deletions test/napi/uv.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ describe.if(!isWindows)("uv stubs", () => {
expect(nativeModule.testUvOnce()).toBe(1);
});

// Regression test for #29223: ffi-napi calls uv_thread_self during NAPI
// module init. Used to panic because it was a stubbed libuv symbol.
test("uv_thread_self (#29223)", () => {
expect(nativeModule.testThreadSelf()).toBe(true);
});

test("hrtime", () => {
const result = nativeModule.testHrtime();

Expand Down
91 changes: 91 additions & 0 deletions test/regression/issue/29223.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// https://github.com/oven-sh/bun/issues/29223
import { beforeAll, describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows, tempDirWithFiles } from "harness";
import path from "node:path";

// Windows uses real libuv; the POSIX-stub code path does not apply there.
describe.if(!isWindows)("issue #29223", () => {
let tempdir: string = "";

// Build the addon once in beforeAll (same pattern as test/napi/uv.test.ts).
beforeAll(async () => {
const addonSource = `
#include <node_api.h>
#include <pthread.h>
#include <uv.h>

napi_value Init(napi_env env, napi_value exports) {
// This is what ffi-napi does: call uv_thread_self() while the NAPI
// module is being constructed. Before the fix this panicked Bun.
uv_thread_t self = uv_thread_self();

// Also check that calling it twice from the same thread agrees with
// pthread_self() — proves we actually implemented it rather than
// returning a garbage value.
uv_thread_t again = uv_thread_self();
int equal = pthread_equal(self, again) != 0 && pthread_equal(self, pthread_self()) != 0;

napi_value equal_js;
napi_get_boolean(env, equal, &equal_js);
napi_set_named_property(env, exports, "equal", equal_js);
return exports;
}

NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)
`;

const files = {
"addon.c": addonSource,
"package.json": JSON.stringify({
name: "issue-29223-addon",
version: "0.0.0",
private: true,
scripts: { "build:napi": "node-gyp configure && node-gyp build" },
dependencies: { "node-gyp": "10.2.0" },
}),
"binding.gyp": `{
"targets": [
{
"target_name": "addon",
"sources": [ "addon.c" ],
"include_dirs": [ ".", "./libuv" ],
"cflags": ["-fPIC"]
}
Comment thread
robobun marked this conversation as resolved.
]
}`,
"index.js": `const addon = require("./build/Release/addon.node");
if (addon.equal !== true) {
console.error("FAIL: uv_thread_self returned inconsistent results");
process.exit(2);
}
console.log("OK");
`,
};

tempdir = tempDirWithFiles("issue-29223", files);

// node-gyp uses the libuv headers we vendor for the stubs.
const libuvDir = path.join(import.meta.dir, "../../../src/bun.js/bindings/libuv");
await Bun.$`cp -R ${libuvDir} ${path.join(tempdir, "libuv")}`.env(bunEnv);
await Bun.$`${bunExe()} install && ${bunExe()} run build:napi`.env(bunEnv).cwd(tempdir);
// cold `bun install` + node-gyp build can exceed the default 5s hook timeout on CI
}, 60_000);

Check warning on line 72 in test/regression/issue/29223.test.ts

View check run for this annotation

Claude / Claude Code Review

beforeAll timeout 60_000 still violates no-timeout convention

The `beforeAll` in `test/regression/issue/29223.test.ts` still closes with `}, 60_000);` (line 72), violating CLAUDE.md's rule: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The previous review comment flagged the original 120,000 ms; commit c5db337 reduced it to 60,000 ms instead of removing it. The companion file `test/napi/uv.test.ts` (added in the same PR, running the identical node-gyp build in its own `beforeAll`) correctly omits the timeout argument, making the two
Comment thread
robobun marked this conversation as resolved.
Outdated

test("NAPI addon calling uv_thread_self during Init does not crash", () => {
// spawnSync because the baseline (pre-fix) crashes via panic + abort;
// spawn + proc.exited can hang on such aborts under the test runner.
const { stdout, exitCode } = Bun.spawnSync({
cmd: [bunExe(), "index.js"],
env: bunEnv,
cwd: tempdir,
stderr: "pipe",
stdout: "pipe",
});

// The addon prints "OK" from index.js only if require() succeeded and
// uv_thread_self() returned a thread id consistent with pthread_self().
// Pre-fix, require() panics the process and stdout stays empty.
expect(stdout.toString().trim()).toBe("OK");
expect(exitCode).toBe(0);
});
});
Loading