Bug 299242
| Summary: | [JSC] Math.random JIT path uses arithmetic instead of logical right shifts | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | oze41998 |
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Trivial | CC: | webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | Safari 18 | ||
| Hardware: | Mac (Intel) | ||
| OS: | macOS 15 | ||
oze41998
This report concerns Math.random output when taking the JIT path.
Currently, state advancement in emitRandomThunkImpl (Source/JavaScriptCore/jit/AssemblyHelpers.cpp, around line 847) uses arithmetic right shifts (rshift64), whereas all other advancement paths use logical right shifts.
Reference implementation (WeakRandom):
WeakRandom::nextState uses the >> operator on a uint64_t, which performs a logical shift. Source: Source/WTF/wtf/WeakRandom.h, line 109.
FTL lowering path:
compileArithRandom uses lShr (logical shift right). Source: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp, line 3501.
JIT path (problem):
emitRandomThunkImpl calls jit.rshift64, which emits SAR (arithmetic shift). Source: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h, line 5311.
By contrast, urshift64 emits SHR (logical shift). Source: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h, line 5350. This matches the intended semantics.
Impact:
This discrepancy means that Math.random produces different output depending on whether the JIT path is taken. The bug is not immediately obvious because both arithmetic and logical right shifts still generate values within the expected [0, 1) range, and the distribution may look reasonable at a glance. However, the underlying state advancement diverges from the intended algorithm, which leads to inconsistent and incorrect behavior compared to other code paths.
Proposed fix:
Replace calls to rshift64 with urshift64 inside emitRandomThunkImpl.
A pull request for this fix will follow.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
oze41998
PR https://github.com/WebKit/WebKit/pull/51077
Radar WebKit Bug Importer
<rdar://problem/161480043>
Radar WebKit Bug Importer
<rdar://problem/161479945>
EWS
Committed 300785@main (cf0e17e68442): <https://commits.webkit.org/300785@main>
Reviewed commits have been landed. Closing PR #51077 and removing active labels.