Skip to content

fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 2 commits into from
Dec 17, 2025
Merged

fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46c51a2
fix: v8::Value::IsInt32()/IsUint32() edge cases
190n Dec 17, 2025
7217f3a
[autofix.ci] apply automated fixes
autofix-ci[bot] Dec 17, 2025
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
4 changes: 2 additions & 2 deletions src/bun.js/bindings/v8/V8Value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ bool Value::IsNumber() const

bool Value::IsUint32() const
{
return localToJSValue().isUInt32();
return localToJSValue().isUInt32AsAnyInt();
}

bool Value::IsUndefined() const
Expand Down Expand Up @@ -81,7 +81,7 @@ bool Value::IsArray() const

bool Value::IsInt32() const
{
return localToJSValue().isInt32();
return localToJSValue().isInt32AsAnyInt();
}

bool Value::IsBigInt() const
Expand Down
6 changes: 3 additions & 3 deletions test/v8/v8-module/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,21 +1120,21 @@ void test_v8_value_type_checks(const FunctionCallbackInfo<Value> &info) {
}

Local<Value> value = info[0];

// Test all type checks
printf("IsMap: %s\n", value->IsMap() ? "true" : "false");
printf("IsArray: %s\n", value->IsArray() ? "true" : "false");
printf("IsInt32: %s\n", value->IsInt32() ? "true" : "false");
printf("IsBigInt: %s\n", value->IsBigInt() ? "true" : "false");

// Also test some existing checks for comparison
printf("IsNumber: %s\n", value->IsNumber() ? "true" : "false");
printf("IsUint32: %s\n", value->IsUint32() ? "true" : "false");
printf("IsObject: %s\n", value->IsObject() ? "true" : "false");
printf("IsBoolean: %s\n", value->IsBoolean() ? "true" : "false");
printf("IsString: %s\n", value->IsString() ? "true" : "false");
printf("IsFunction: %s\n", value->IsFunction() ? "true" : "false");

return ok(info);
}

Expand Down
17 changes: 2 additions & 15 deletions test/v8/v8-module/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,15 @@ const buildMode = process.argv[5];

const tests = require("./module")(buildMode === "debug");

// Custom JSON reviver to handle BigInt
function parseArgs(str) {
return JSON.parse(str, (_, value) =>
value && typeof value === "object" && "__bigint__" in value ? BigInt(value.__bigint__) : value,
);
}

const testName = process.argv[2];
const args = parseArgs(process.argv[3] ?? "[]");
const args = eval(process.argv[3] ?? "[]");

@coderabbitai coderabbitai Bot Dec 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Intentional use of eval() for test expressions—consider adding a comment.

The switch from JSON.parse() to eval() is necessary to support test arguments containing JavaScript expressions like Math.fround(1) that cannot be represented in JSON. Since this is a test harness file with controlled inputs (only invoked by the test suite), the security risk is acceptable.

Consider adding a brief comment explaining why eval() is used:

-const args = eval(process.argv[3] ?? "[]");
+// eval() is needed to support JS expressions (e.g., Math.fround(1)) that JSON cannot represent
+const args = eval(process.argv[3] ?? "[]");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const args = eval(process.argv[3] ?? "[]");
// eval() is needed to support JS expressions (e.g., Math.fround(1)) that JSON cannot represent
const args = eval(process.argv[3] ?? "[]");
🧰 Tools
🪛 Biome (2.1.2)

[error] 9-9: eval() exposes to security risks and performance issues.

See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().

(lint/security/noGlobalEval)

🤖 Prompt for AI Agents 
In test/v8/v8-module/main.js around line 9, the use of eval(process.argv[3] ??
"[]") is intentional but lacks an explanatory comment; add a brief comment
immediately above this line stating that eval is used intentionally to allow
JavaScript expressions (e.g., Math.fround(1)) in test arguments, that inputs are
controlled by the test suite, and that this is not a security risk in this
context, leaving the eval call unchanged.

const thisValue = JSON.parse(process.argv[4] ?? "null");

function runGC() {
if (typeof Bun !== "undefined") {
Bun.gc(true);
}
}

const fn = tests[testName];
if (typeof fn !== "function") {
throw new Error("Unknown test:", testName);
}
const result = fn.apply(thisValue, [runGC, ...args]);
const result = fn.apply(thisValue, [...args]);
if (result) {
console.log(result == global);
throw new Error(result);
Expand Down
124 changes: 65 additions & 59 deletions test/v8/v8.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { spawn } from "bun";
import { jscDescribe } from "bun:jsc";
import { beforeAll, describe, expect, it } from "bun:test";
import { bunEnv, bunExe, isASAN, isBroken, isMusl, isWindows, nodeExe, tmpdirSync } from "harness";
import assert from "node:assert";
Expand Down Expand Up @@ -124,135 +125,141 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => {

describe("module lifecycle", () => {
it("can call a basic native function", async () => {
await checkSameOutput("test_v8_native_call", []);
await checkSameOutput("test_v8_native_call");
});
});

describe("primitives", () => {
it("can create and distinguish between null, undefined, true, and false", async () => {
await checkSameOutput("test_v8_primitives", []);
await checkSameOutput("test_v8_primitives");
});
});

describe("Value type checks", () => {
it("matches Node for IsMap/IsArray/IsInt32/IsBigInt on representative values", async () => {
// Each entry is an argument list; we wrap each value so the addon receives it as info[0].
const cases: any[][] = [
[new Map()],
[[]],
[42],
[2147483647], // INT32_MAX
[2147483648], // INT32_MAX + 1 (should not be Int32)
[-2147483648], // INT32_MIN
[-2147483649], // INT32_MIN - 1 (should not be Int32)
[123n],
[3.14],
["string"],
[{}],
];
for (const args of cases) {
await checkSameOutput("test_v8_value_type_checks", args);
}
it("Math.fround returns a double-encoded value", () => {
// If this fails, you need to find a new way to make a JSValue which uses the double encoding
// but holds an int32 value (maybe Float64Array?)
expect(jscDescribe(Math.fround(1))).toBe("Double: 4607182418800017408, 1.000000");
});
Comment on lines +139 to +143

@coderabbitai coderabbitai Bot Dec 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and examine the test file around the Math.fround test
find . -name "v8.test.ts" -type f 2>/dev/null | head -5

# Look at the test file content, especially around lines 139-143
if [ -f "test/v8/v8.test.ts" ]; then
  echo "=== File content around Math.fround test ==="
  sed -n '120,160p' test/v8/v8.test.ts
  
  echo -e "\n=== Look for imports and jscDescribe usage ==="
  head -50 test/v8/v8.test.ts
  
  echo -e "\n=== Search for other tests using jscDescribe ==="
  grep -n "jscDescribe" test/v8/v8.test.ts | head -20
fi

Repository: oven-sh/bun

Length of output: 3362

Add Node.js comparison or clarify test intent. This test uses only jscDescribe() without comparing against Node.js output, breaking the pattern of other V8 tests in the same file that use checkSameOutput(). Either add a Node.js comparison via checkSameOutput() to validate the double-encoded behavior is consistent, or move this to a JSC-specific test if it's intentionally testing only JavaScriptCore's internal representation. The existing comment acknowledges brittleness but doesn't address why this test diverges from the established V8 testing pattern.

🤖 Prompt for AI Agents 
In test/v8/v8.test.ts around lines 139 to 143, the Math.fround test uses only
jscDescribe() and breaks the pattern of other V8 tests that call
checkSameOutput(); update this test to either call checkSameOutput() with an
equivalent Node.js invocation to verify the double-encoded behavior matches
Node, or, if the intent is to validate a JSC-specific internal representation,
move the test into a JSC-specific test file and keep it as-is but
rename/annotate it to indicate it is JSC-only; implement the change by replacing
the expect(jscDescribe(...)) assertion with a checkSameOutput() call that runs
the same expression under Node.js and JSC, or relocating the test and updating
test suite organization and comments accordingly.


it.each([
// Each entry should eval() to an array of arguments
"[new Map()]",
"[[]]",
"[42]",
"[2 ** 31 - 1]", // INT32_MAX
"[2 ** 31]", // INT32_MAX + 1 (should not be Int32)
"[-(2 ** 31)]", // INT32_MIN
"[-(2 ** 31) - 1]", // INT32_MIN - 1 (should not be Int32)
"[2 ** 32 - 1]", // UINT32_MAX
"[2 ** 32]", // UINT32_MAX + 1
"[Math.fround(1)]", // Value represented as a double but whose numeric value fits in the int32 range (should be int32)
"[123n]",
"[3.14]",
"['string']",
"[{}]",
])("matches Node for IsMap/IsArray/IsInt32/IsBigInt on %s", async args => {
await checkSameOutput("test_v8_value_type_checks", args);
});
});
describe("Number", () => {
it("can create small integer", async () => {
await checkSameOutput("test_v8_number_int", []);
await checkSameOutput("test_v8_number_int");
});
// non-i32 v8::Number is not implemented yet
it("can create large integer", async () => {
await checkSameOutput("test_v8_number_large_int", []);
await checkSameOutput("test_v8_number_large_int");
});
it("can create fraction", async () => {
await checkSameOutput("test_v8_number_fraction", []);
await checkSameOutput("test_v8_number_fraction");
});
});

describe("String", () => {
it("can create and read back strings with only ASCII characters", async () => {
await checkSameOutput("test_v8_string_ascii", []);
await checkSameOutput("test_v8_string_ascii");
});
// non-ASCII strings are not implemented yet
it("can create and read back strings with UTF-8 characters", async () => {
await checkSameOutput("test_v8_string_utf8", []);
await checkSameOutput("test_v8_string_utf8");
});
it("handles replacement correctly in strings with invalid UTF-8 sequences", async () => {
await checkSameOutput("test_v8_string_invalid_utf8", []);
await checkSameOutput("test_v8_string_invalid_utf8");
});
it("can create strings from null-terminated Latin-1 data", async () => {
await checkSameOutput("test_v8_string_latin1", []);
await checkSameOutput("test_v8_string_latin1");
});
describe("WriteUtf8", () => {
it("truncates the string correctly", async () => {
await checkSameOutput("test_v8_string_write_utf8", []);
await checkSameOutput("test_v8_string_write_utf8");
});
});
});

describe("External", () => {
it("can create an external and read back the correct value", async () => {
await checkSameOutput("test_v8_external", []);
await checkSameOutput("test_v8_external");
});
});

describe("Value", () => {
it("can compare values using StrictEquals", async () => {
await checkSameOutput("test_v8_strict_equals", []);
await checkSameOutput("test_v8_strict_equals");
});
});

describe("Object", () => {
it("can create an object and set properties", async () => {
await checkSameOutput("test_v8_object", []);
await checkSameOutput("test_v8_object");
});
it("can get properties by key using Object::Get(context, key)", async () => {
await checkSameOutput("test_v8_object_get_by_key", []);
await checkSameOutput("test_v8_object_get_by_key");
});
it("can get array elements by index using Object::Get(context, index)", async () => {
await checkSameOutput("test_v8_object_get_by_index", []);
await checkSameOutput("test_v8_object_get_by_index");
});
it("correctly handles exceptions from get and set", async () => {
await checkSameOutput("test_v8_object_get_set_exceptions", []);
await checkSameOutput("test_v8_object_get_set_exceptions");
});
});
describe("Array", () => {
it("can create an array from a C array of Locals", async () => {
await checkSameOutput("test_v8_array_new", []);
await checkSameOutput("test_v8_array_new");
});
it("can create an array with a specific length", async () => {
await checkSameOutput("test_v8_array_new_with_length", []);
await checkSameOutput("test_v8_array_new_with_length");
});
it("can create an array from a callback", async () => {
await checkSameOutput("test_v8_array_new_with_callback", []);
await checkSameOutput("test_v8_array_new_with_callback");
});
it("correctly reports array length", async () => {
await checkSameOutput("test_v8_array_length", []);
await checkSameOutput("test_v8_array_length");
});
it("can iterate over array elements with callbacks", async () => {
await checkSameOutput("test_v8_array_iterate", []);
await checkSameOutput("test_v8_array_iterate");
});
});

describe("ObjectTemplate", () => {
it("creates objects with internal fields", async () => {
await checkSameOutput("test_v8_object_template", []);
await checkSameOutput("test_v8_object_template");
});
});

describe("FunctionTemplate", () => {
it("keeps the data parameter alive", async () => {
await checkSameOutput("test_v8_function_template", []);
await checkSameOutput("test_v8_function_template");
});
});

describe("Function", () => {
it("correctly receives all its arguments from JS", async () => {
await checkSameOutput("print_values_from_js", [5.0, true, null, false, "async meow", {}]);
await checkSameOutput("print_native_function", []);
await checkSameOutput("print_values_from_js", "[5.0, true, null, false, 'async meow', {}]");
await checkSameOutput("print_native_function");
});

it("correctly receives the this value from JS", async () => {
await checkSameOutput("call_function_with_weird_this_values", []);
await checkSameOutput("call_function_with_weird_this_values");
});
});

Expand All @@ -272,51 +279,51 @@ describe.todoIf(isBroken && isMusl)("node:v8", () => {

describe("Global", () => {
it("can create, modify, and read the value from global handles", async () => {
await checkSameOutput("test_v8_global", []);
await checkSameOutput("test_v8_global");
});
});

describe("HandleScope", () => {
it("can hold a lot of locals", async () => {
await checkSameOutput("test_many_v8_locals", []);
await checkSameOutput("test_many_v8_locals");
});
// Skip on ASAN: false positives due to dynamic library boundary crossing where
// Bun is built with ASAN+UBSAN but the native addon is not
it.skipIf(isASAN)(
"keeps GC objects alive",
async () => {
await checkSameOutput("test_handle_scope_gc", []);
await checkSameOutput("test_handle_scope_gc");
},
10000,
);
});

describe("EscapableHandleScope", () => {
it("keeps handles alive in the outer scope", async () => {
await checkSameOutput("test_v8_escapable_handle_scope", []);
await checkSameOutput("test_v8_escapable_handle_scope");
});
});

describe("MaybeLocal", () => {
it("correctly handles ToLocal and ToLocalChecked operations", async () => {
await checkSameOutput("test_v8_maybe_local", []);
await checkSameOutput("test_v8_maybe_local");
});
});

describe("uv_os_getpid", () => {
it.skipIf(isWindows)("returns the same result as getpid on POSIX", async () => {
await checkSameOutput("test_uv_os_getpid", []);
await checkSameOutput("test_uv_os_getpid");
});
});

describe("uv_os_getppid", () => {
it.skipIf(isWindows)("returns the same result as getppid on POSIX", async () => {
await checkSameOutput("test_uv_os_getppid", []);
await checkSameOutput("test_uv_os_getppid");
});
});
});

async function checkSameOutput(testName: string, args: any[], thisValue?: any) {
async function checkSameOutput(testName: string, args?: string, thisValue?: any) {
const [nodeResultResolution, bunReleaseResultResolution, bunDebugResultResolution] = await Promise.allSettled([
runOn(Runtime.node, BuildMode.release, testName, args, thisValue),
runOn(Runtime.bun, BuildMode.release, testName, args, thisValue),
Expand Down Expand Up @@ -344,12 +351,11 @@ async function checkSameOutput(testName: string, args: any[], thisValue?: any) {
return nodeResult;
}

// Custom JSON serialization that handles BigInt
function serializeArgs(args: any[]): string {
return JSON.stringify(args, (_, value) => (typeof value === "bigint" ? { __bigint__: value.toString() } : value));
}

async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, jsArgs: any[], thisValue?: any) {
/**
* @param jsArgs should eval() to an array
* @param thisValue will be JSON stringified
*/
async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, jsArgs?: string, thisValue?: any) {
if (runtime == Runtime.node) {
assert(buildMode == BuildMode.release);
}
Expand All @@ -366,7 +372,7 @@ async function runOn(runtime: Runtime, buildMode: BuildMode, testName: string, j
...(runtime == Runtime.bun ? ["--smol"] : []),
join(baseDir, "main.js"),
testName,
serializeArgs(jsArgs),
jsArgs ?? "[]",
JSON.stringify(thisValue ?? null),
];
if (buildMode == BuildMode.debug) {
Expand Down