Skip to content

Fix conversions from JSValue to FFI pointer#25045

Merged
taylordotfish merged 4 commits into
mainfrom
taylor.fish/fix-ffi-jsvalue-ptr
Nov 25, 2025
Merged

Fix conversions from JSValue to FFI pointer#25045
taylordotfish merged 4 commits into
mainfrom
taylor.fish/fix-ffi-jsvalue-ptr

Conversation

@taylordotfish

@taylordotfish taylordotfish commented Nov 24, 2025

Copy link
Copy Markdown
Contributor

Fixes this issue, where two identical JS numbers could become two different FFI pointers:

// gcc -fpic -shared -o main.so
#include <stdio.h>

void* getPtr(void) {
    return (void*)123;
}

void printPtr(void* ptr) {
    printf("%zu\n", (size_t)ptr);
}
import { dlopen, FFIType } from "bun:ffi";

const lib = dlopen("./main.so", {
  getPtr: { args: [], returns: FFIType.ptr },
  printPtr: { args: [FFIType.ptr], returns: FFIType.void },
});

const ptr = lib.symbols.getPtr();
console.log(`${typeof ptr} ${ptr}`);

const ptr2 = Number(String(ptr));
console.log(`${typeof ptr2} ${ptr2}`);

console.log(`pointers equal? ${ptr === ptr2}`);
lib.symbols.printPtr(ptr);
lib.symbols.printPtr(ptr2);
$ bun main.js
number 123
number 123
pointers equal? true
123
18446744073709551615

Fixes #20072

(For internal tracking: fixes ENG-22327)

@robobun

robobun commented Nov 24, 2025

Copy link
Copy Markdown
Collaborator
Updated 2:34 PM PT - Nov 24th, 2025

@taylordotfish, your commit 39e3bc4 has 3 failures in Build #32341 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 25045

That installs a local version of the PR into your bun-25045 executable, so you can run:

bun-25045 --bun

@coderabbitai

coderabbitai Bot commented Nov 24, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

C++ JSValue pointer encoding/decoding was modified to use uintptr_t and to explicitly handle NumberTag-encoded int32 values; corresponding Zig helpers that converted pointers↔EncodedJSValue were removed, requiring callers to stop using those two inline functions.

Changes

Cohort / File(s) Summary
C++ FFI pointer encoding updates
src/bun.js/api/FFI.h
Reworked pointer encoding/decoding to use uintptr_t for pointer-width correctness, added explicit handling of NumberTag-encoded INT32 values in JSVALUE_TO_PTR, replaced previous size_t-based double casts with uintptr_t↔double casts, and minor brace/formatting adjustments in null handling.
Zig FFI binding removal
src/bun.js/bindings/FFI.zig
Removed pub inline fn PTR_TO_JSVALUE(arg_ptr: ?*anyopaque) EncodedJSValue and pub inline fn JSVALUE_TO_PTR(arg_val: EncodedJSValue) ?*anyopaque, eliminating Zig-level pointer↔EncodedJSValue helpers; other JSValue conversion helpers unchanged.

Possibly related PRs

Suggested reviewers

  • dylan-conway
  • nektro

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive PR description provides clear issue context with code examples but lacks verification details on how the fix was tested. Add a 'How did you verify your code works?' section describing testing approach, test cases, and verification results for the pointer conversion fix.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing conversions between JSValue and FFI pointers, which is directly evidenced by the modifications to pointer encoding/decoding logic in FFI.h and the removal of conversion functions in FFI.zig.

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0da132e and 17f9c63.

📒 Files selected for processing (2)
  • src/bun.js/api/FFI.h (1 hunks)
  • src/bun.js/bindings/FFI.zig (0 hunks)
💤 Files with no reviewable changes (1)
  • src/bun.js/bindings/FFI.zig
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:458-475
Timestamp: 2025-10-18T20:59:45.579Z
Learning: In src/bun.js/telemetry.zig, the RequestId (u64) to JavaScript number (f64) conversion in jsRequestId() is intentionally allowed to lose precision beyond 2^53-1. This is acceptable because: (1) at 1M requests/sec it takes ~285 years to overflow, (2) the counter resets per-process, and (3) these are observability IDs, not critical distributed IDs. Precision loss is an acceptable trade-off for this use case.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.196Z
Learning: Applies to **/js_*.zig : Implement proper memory management with reference counting using `ref()`/`deref()` in JavaScript bindings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.196Z
Learning: Applies to **/js_*.zig : Use `bun.JSError!JSValue` for proper error propagation in JavaScript bindings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.548Z
Learning: Applies to **/*.zig : Use `bun.JSError!JSValue` return type for Zig methods and constructors to enable proper error handling and exception propagation
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.866Z
Learning: Applies to *.cpp : To create JavaScript objects from Zig, implement C++ functions following the `Bun__ClassName__toJS(Zig::GlobalObject*, NativeType*)` convention that construct and return the JavaScript object as an encoded JSValue
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.675Z
Learning: Use tagged pointers for efficient V8 value representation where small integers are stored directly as Smis and objects use pointer tagging with map pointers
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.548Z
Learning: Applies to **/*.zig : Expose generated bindings in Zig structs using `pub const js = JSC.Codegen.JS<ClassName>` with trait conversion methods: `toJS`, `fromJS`, and `fromJSDirect`
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.866Z
Learning: Applies to *.cpp : Implement getter functions using `JSC_DEFINE_CUSTOM_GETTER` macro, performing type checking with `jsDynamicCast`, throwing errors for type mismatches, and returning encoded JSValue
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.196Z
Learning: Applies to **/*js_bindings.classes.ts : Use `JSC.Codegen` correctly to generate necessary binding code for JavaScript-Zig integration
📚 Learning: 2025-10-15T20:19:37.256Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23680
File: cmake/targets/BuildBun.cmake:822-822
Timestamp: 2025-10-15T20:19:37.256Z
Learning: In the Bun codebase, FFI (Foreign Function Interface) code is compiled separately using TinyCC (tcc), which barely supports C99. Headers like src/bun.js/api/FFI.h and src/bun.js/api/ffi-stdbool.h are only used for FFI compilation with tcc, not with the main compiler. Therefore, C standard changes to the main Bun target do not affect FFI code compilation.

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-10-15T20:19:38.580Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23680
File: cmake/targets/BuildBun.cmake:822-822
Timestamp: 2025-10-15T20:19:38.580Z
Learning: In the Bun codebase, FFI is compiled with tcc (TinyCC), which barely supports C99. The headers `src/bun.js/api/FFI.h` and `src/bun.js/api/ffi-stdbool.h` are only used for FFI compilation with tcc, not for the main Bun target. Therefore, C23 compatibility concerns (such as bool/true/false keyword conflicts) do not apply to these FFI headers.

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:36:59.675Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.675Z
Learning: Use tagged pointers for efficient V8 value representation where small integers are stored directly as Smis and objects use pointer tagging with map pointers

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:36:59.675Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.675Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:36:59.675Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.675Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:37:47.887Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.887Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:36:33.049Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:33.049Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : When implementing JavaScript classes in C++, define properties using HashTableValue arrays and add iso subspaces for classes with C++ fields, caching structures in ZigGlobalObject

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:36:59.675Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.675Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method

Applied to files:

  • src/bun.js/api/FFI.h
📚 Learning: 2025-11-24T18:35:25.866Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-11-24T18:35:25.866Z
Learning: Applies to *.cpp : Implement getter functions using `JSC_DEFINE_CUSTOM_GETTER` macro, performing type checking with `jsDynamicCast`, throwing errors for type mismatches, and returning encoded JSValue

Applied to files:

  • src/bun.js/api/FFI.h
🔇 Additional comments (2)
src/bun.js/api/FFI.h (2)

227-229: LGTM: Proper use of uintptr_t for pointer conversions.

The typed array early return is correctly placed, and the change from size_t to uintptr_t on line 236 is the appropriate type for pointer-to-integer conversions, avoiding potential undefined behavior.

Also applies to: 236-236


242-247: LGTM: Consistent uintptr_t usage in pointer encoding.

The null pointer handling remains correct, and the use of uintptr_t on line 247 maintains consistency with the inverse conversion in JSVALUE_TO_PTR, ensuring proper bidirectional pointer-integer conversions.

Comment thread src/bun.js/api/FFI.h Outdated
@linear

linear Bot commented Nov 24, 2025

Copy link
Copy Markdown

@taylordotfish taylordotfish merged commit 7335cb7 into main Nov 25, 2025
59 of 61 checks passed
@taylordotfish taylordotfish deleted the taylor.fish/fix-ffi-jsvalue-ptr branch November 25, 2025 01:34
@coderabbitai coderabbitai Bot mentioned this pull request Nov 29, 2025
@nektro nektro mentioned this pull request Dec 2, 2025
robobun added a commit that referenced this pull request Apr 15, 2026
Covers the pattern from issue #29346: an out-parameter writes a handle
into the low 2 GiB, the caller reads it back with `ffiRead.ptr`, and
passes it as a `ptr` FFI argument. That routes through the Int32
JSValue branch of `JSVALUE_TO_PTR` (added in #25045). Before that
branch, the double path sign-extended the Int32 into 0xFFFFFFFFxxxxxxxx
and the callee segfaulted at 0xFFFFFFFFFFFFFFFF.
robobun added a commit that referenced this pull request Apr 16, 2026
Covers the pattern from issue #29346: an out-parameter writes a handle
into the low 2 GiB, the caller reads it back with `ffiRead.ptr`, and
passes it as a `ptr` FFI argument. That routes through the Int32
JSValue branch of `JSVALUE_TO_PTR` (added in #25045). Before that
branch, the double path sign-extended the Int32 into 0xFFFFFFFFxxxxxxxx
and the callee segfaulted at 0xFFFFFFFFFFFFFFFF.
robobun added a commit that referenced this pull request Apr 17, 2026
Covers the pattern from issue #29346: an out-parameter writes a handle
into the low 2 GiB, the caller reads it back with `ffiRead.ptr`, and
passes it as a `ptr` FFI argument. That routes through the Int32
JSValue branch of `JSVALUE_TO_PTR` (added in #25045). Before that
branch, the double path sign-extended the Int32 into 0xFFFFFFFFxxxxxxxx
and the callee segfaulted at 0xFFFFFFFFFFFFFFFF.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi bug?

3 participants