Fix UB in IntegralTypedArrayAdaptor::toNativeFromDouble()#179
Fix UB in IntegralTypedArrayAdaptor::toNativeFromDouble()#179carlsmedstad wants to merge 2 commits intooven-sh:mainfrom
Conversation
static_cast<int32_t>(double) is undefined behavior in C++ when the value is outside [INT32_MIN, INT32_MAX]. The subsequent range check can be legally optimized away by the compiler, causing values > 2^31 stored to typed arrays to silently become 0x80000000 (INT32_MIN). This breaks pure-JS crypto libraries (@noble/hashes, better-auth, etc.) that rely on typed array correctness for bitwise operations. Always use toInt32() unconditionally, matching the existing ARM (FJCVTZS) code path. toInt32() is ALWAYS_INLINE and implements proper ECMA-262 ToInt32 semantics via bit manipulation. First detected on Arch Linux since version 1.3.11, built with Clang 22, triggered the UB. References: - oven-sh/bun#28607 - https://gitlab.archlinux.org/archlinux/packaging/packages/bun/-/commit/e6f882ca
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoved FJCVTZS-specific branching in Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
|
Thanks. Can you also change ALWAYS_INLINE int32_t toInt32(double number)
{
#if HAVE(FJCVTZS_INSTRUCTION)
int32_t result = 0;
__asm__ ("fjcvtzs %w0, %d1" : "=r" (result) : "w" (number) : "cc");
return result;
#elif CPU(X86_64) && COMPILER(GCC_COMPATIBLE)
int32_t result;
__asm__ ("cvttsd2si %1, %0" : "=r" (result) : "x" (number));
if (static_cast<double>(result) != number)
return toIntImpl<int32_t>(number);
return result;
#else
return toIntImpl<int32_t>(number);
#endif
} |
Use inline asm to avoid UB from static_cast<int32_t>(double), falling back to toIntImpl when truncation isn't exact. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@dylan-conway Sure thing, see 3f10db7 |
scrypt was non-deterministic on Arch Linux (Bun built with Clang 22) because
WebKit's IntegralTypedArrayAdaptor::toNativeFromDouble() did:
int32_t result = static_cast<int32_t>(value);
if (static_cast<double>(result) != value) result = toInt32(value);
return static_cast<Type>(result);
static_cast<int32_t>(double) is undefined behavior in C++ when value is
outside [INT32_MIN, INT32_MAX]. Clang 22 legally optimizes the subsequent
range check away, silently turning every out-of-range typed-array store into
INT32_MIN. That corrupted @noble/hashes/scrypt — and therefore Better Auth's
hashPassword — producing different output for the same input across runs.
Picks up oven-sh/WebKit#179, which always routes through toInt32(): on
x86_64 that uses a cvttsd2si inline-asm fast path with a portable fallback,
matching the existing FJCVTZS ARM code path.
Adds a regression test at test/regression/issue/28607.test.ts that exercises
Int32Array / Uint32Array / Int16Array stores of out-of-range values and
verifies the scrypt-style add + rotate + xor mixing pattern is
deterministic.
|
Needed for oven-sh/bun#28607 (scrypt non-determinism on Arch Linux / Clang 22). The Bun-side PR that picks up this fix is oven-sh/bun#28905 — it's blocked on the Friendly ping @Jarred-Sumner @dylan-conway — once the preview build lands I can rebase the Bun PR onto the published SHA and CI will go green. |
scrypt was non-deterministic on Arch Linux (Bun built with Clang 22) because
WebKit's IntegralTypedArrayAdaptor::toNativeFromDouble() did:
int32_t result = static_cast<int32_t>(value);
if (static_cast<double>(result) != value) result = toInt32(value);
return static_cast<Type>(result);
static_cast<int32_t>(double) is undefined behavior in C++ when value is
outside [INT32_MIN, INT32_MAX]. Clang 22 legally optimizes the subsequent
range check away, silently turning every out-of-range typed-array store into
INT32_MIN. That corrupted @noble/hashes/scrypt — and therefore Better Auth's
hashPassword — producing different output for the same input across runs.
Picks up oven-sh/WebKit#179, which always routes through toInt32(): on
x86_64 that uses a cvttsd2si inline-asm fast path with a portable fallback,
matching the existing FJCVTZS ARM code path.
Adds a regression test at test/regression/issue/28607.test.ts that exercises
Int32Array / Uint32Array / Int16Array stores of out-of-range values and
verifies the scrypt-style add + rotate + xor mixing pattern is
deterministic.
The Bun-side fix for #28607 requires oven-sh/WebKit#179 to merge (or its Preview Build workflow to be approved so the prebuilt binary publishes). Neither has happened yet, and pointing WEBKIT_VERSION at a non-existent preview tag breaks the prebuilt fetch on every CI build lane. Keep the regression test at test/regression/issue/28607.test.ts so the bug's observable behavior is captured. Once oven-sh/WebKit#179 lands and a WebKit version bump picks it up, this test will guard against re-regression.
static_cast<int32_t>(double) is undefined behavior in C++ when the value is outside [INT32_MIN, INT32_MAX]. The subsequent range check can be legally optimized away by the compiler, causing values > 2^31 stored to typed arrays to silently become 0x80000000 (INT32_MIN).
This breaks pure-JS crypto libraries (@noble/hashes, better-auth, etc.) that rely on typed array correctness for bitwise operations.
Always use toInt32() unconditionally, matching the existing ARM (FJCVTZS) code path. toInt32() is ALWAYS_INLINE and implements proper ECMA-262 ToInt32 semantics via bit manipulation.
First detected on Arch Linux since version 1.3.11, built with Clang 22, triggered the UB.
References: