Skip to content

Add regression test for scrypt non-determinism (#28607)#28905

Draft
robobun wants to merge 4 commits intomainfrom
farm/271f8c56/fix-28607-typed-array-int32-ub
Draft

Add regression test for scrypt non-determinism (#28607)#28905
robobun wants to merge 4 commits intomainfrom
farm/271f8c56/fix-28607-typed-array-int32-ub

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented Apr 6, 2026

Regression test for #28607. The actual fix is oven-sh/WebKit#179 — which needs to merge upstream before this bug is closed. See below for why the Bun-side change is just a test.

Repro

import { hashPassword, verifyPassword } from 'better-auth/crypto';
const password = 'TestPassword123!';
const hash = await hashPassword(password);
await verifyPassword({ hash, password }); // intermittently false on Bun 1.3.11 (Arch)

Root cause

Undefined behavior in WebKit's IntegralTypedArrayAdaptor::toNativeFromDouble() (Source/JavaScriptCore/runtime/TypedArrayAdaptors.h):

static Type toNativeFromDouble(double value)
{
    int32_t result = static_cast<int32_t>(value);          // UB when value > INT32_MAX
    if (static_cast<double>(result) != value)
        result = toInt32(value);
    return static_cast<Type>(result);
}

static_cast<int32_t>(double) is UB 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 corrupts @noble/hashes/scrypt — and therefore Better Auth's hashPassword — producing different output for the same input across runs.

Diagnosed upstream by @carlsmedstad (Arch Linux maintainer) in #28607.

Fix

The fix lives in oven-sh/WebKit#179, which always routes through toInt32():

  • On x86_64 it uses a cvttsd2si inline-asm fast path with a portable fallback to toIntImpl<int32_t>() when truncation isn't exact — avoiding the UB entirely.
  • On ARM it keeps the existing FJCVTZS code path (which was already correct).
  • The dead UB branch in TypedArrayAdaptors.h is removed.

This PR does not bump WEBKIT_VERSION. The earlier attempt (preview-pr-179-3f10db79) broke every C++ build lane because the WebKit Preview Build workflow on that PR is still in action_required state (the PR is from a fork) — no prebuilt binary exists. A WebKit maintainer needs to click Approve and run on https://github.com/oven-sh/WebKit/actions/runs/23983158093 before the tag publishes. Once that lands and oven-sh/WebKit#179 merges to main, a regular WebKit bump will pick up the fix and this test will guard it.

Test

test/regression/issue/28607.test.ts exercises the JS-visible behavior so the regression is caught by CI the next time a clang version triggers the UB:

  • Int32Array stores of values > 2**31 must match v | 0 (ECMA-262 ToInt32 evaluated via a different WebKit code path).
  • Uint32Array stores of values > 2**32 must match v >>> 0.
  • Int16Array stores of values > 2**15 must wrap mod 2^16 with sign adjustment.
  • The same inputs must produce the same Int32Array outputs across 50 repeated runs.
  • A Salsa20-style add + rotate + xor mixing pattern on a 16-lane Int32Array (the exact shape that @noble/hashes/scrypt uses) must be deterministic across 50 runs.

Local bun bd test test/regression/issue/28607.test.ts passes all 5 tests / 54 expects.

Staying draft until WebKit PR #179 is merged so a version bump can pick it up.

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 6, 2026

Updated 1:44 AM PT - Apr 6th, 2026

@robobun, your commit 6a10324 has 2 failures in Build #43914 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 28905

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

bun-28905 --bun

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.
@robobun robobun force-pushed the farm/271f8c56/fix-28607-typed-array-int32-ub branch from dc1771f to 93c8131 Compare April 6, 2026 05:08
autofix-ci bot and others added 2 commits April 6, 2026 05:10
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.
@robobun robobun changed the title Bump WebKit to fix Int32Array UB breaking scrypt on Clang 22 (#28607) Add regression test for scrypt non-determinism (#28607) Apr 6, 2026
Previous gate run hit a compile error in bindings.cpp because an earlier
attempt to seed the WebKit build cache left fc9f2fa7 header files under
the c2010c47 cache slot — JSPromiseReaction.h is a c2010c47-only header.
Cache has been re-extracted from the correct prebuilt tarball.

The PR itself is unchanged: test/regression/issue/28607.test.ts only.
WEBKIT_VERSION stays at main's default (c2010c47). Pushing an empty
commit to re-run the gate.
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.

1 participant