2049714 - Consider canonicalizing NaNs by default in JS::Value setDouble/setNumber

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

LLMs have recently found a number of NaN-canonicalization issues in browser code.

It's time to consider doing one of:

• Canonicalize doubles stored in JS::Value by default. We could add an Unsafe/Unchecked version that doesn't canonicalize.
• Change the isDouble() assertion in setDouble() to a release assertion.

Also note that JS_NumberValue canonicalizes, but JS::NumberValue doesn't; this distinction is not obvious at all.

Maybe we could start by measuring the performance overhead on some micro-benchmarks and Speedometer3/JetStream3.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

How much does this overlap with Bug 932767?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #1)

How much does this overlap with Bug 932767?

Mostly unrelated. Bug 932767 is improving NaN canonicalization for typed array loads with a JIT optimization pass and this bug is about the JS::Value API.

Comment 3

[Andrew McCreight [:mccr8]]

There's also a lot of calls to JS::NumberValue which could potentially be a problem. For instance, I found this one in Canvas WebGL. It looks to me like that's just a double that comes in from the graphics driver or something and it won't get canonicalized if it is NaN AFAICT.

Comment 4

[Jan de Mooij [:jandem]]

I ran some micro-benchmarks for non-JIT-inlined Math.sqrt and Math.atan. NaN-canonicalization is pretty cheap on my Intel machine, but only if the NaN-check is inlined and optimized by Clang. std::isnan isn't inlined by Clang due to our pinned glibc so I changed JS::CanonicalizeNaN to use d != d instead.

I think we should do this, but I want to wait until bug 2050657 is fixed to avoid drawing attention to this with try pushes.

The plan would be roughly:

1. Canonicalize by default for JS::Value.
2. Make sure setNumber doesn't canonicalize for uint32_t etc inputs.
3. Remove now-redundant NaN-canonicalizations that flow into these functions. Remove the confusing JS_NumberValue that already canonicalizes today.
4. Measure benchmark performance. Consider adding accessors that don't canonicalize and use those inside SpiderMonkey where it matters.

Comment 5

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #4)

NaN-canonicalization is pretty cheap on my Intel machine, [...]

That matches my observations when doing µ-benchmarks for the changes in bug 932767: NaN-canonicalization is cheap in most cases because of the branch predictor. But when I tested inputs which were alternating between non-NaN and NaN inputs to thwart branch prediction, omitting NaN-canonicalization through bug 932767 clearly did pay off. (And in addition to that, omitting NaN-canonicalization also leads to fewer instructions, which helps code density.)

FTR: Using d != d will probably require disabling the "NaNExprChecker.h" clang-plugin.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #5)

That matches my observations when doing µ-benchmarks for the changes in bug 932767: NaN-canonicalization is cheap in most cases because of the branch predictor. But when I tested inputs which were alternating between non-NaN and NaN inputs to thwart branch prediction, omitting NaN-canonicalization through bug 932767 clearly did pay off. (And in addition to that, omitting NaN-canonicalization also leads to fewer instructions, which helps code density.)

Optimizing canonicalization in the JIT and perf-sensitive C++ code (the interpreter, Math builtins?) makes a lot of sense. The current JS::Value default behavior is a footgun though, especially for browser/IPC code where engineers are less familiar with JS::Value internals. Non-canonical NaNs are very rare in these cases so we don't really see these bugs in crash reports, but if an attacker can trigger it it's an instant type confusion.

This will need perf numbers for at least sp3 and js3, but with an escape hatch to skip canonicalization where performance really matters, I think it's a good trade-off. For code density it helps that the generated WebIDL bindings already use JS_NumberValue for floating point types so there should be no change there.

Comment 7

[André Bargull [:anba]]

I fully agree with that. My comment was just aimed at supporting your observation that NaN canonicalization is cheap, because I've observed the same when testing for bug 932767.

Related: If JS::NumberValue canonicalizes by default and WebIDL bindings are changed to use JS::NumberValue instead of JS_NumberValue, it could actually improve performance for int64_t/uint64_t values, which are currently first converted to double and then passed to JS_NumberValue, which calls mozilla::NumberIsInt32 to detect Int32 representable values. JS::NumberValue performs this Int32-valued check with simple branches avoiding to call mozilla::NumberIsInt32.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #7)

Related: If JS::NumberValue canonicalizes by default and WebIDL bindings are changed to use JS::NumberValue instead of JS_NumberValue, it could actually improve performance for int64_t/uint64_t values, which are currently first converted to double and then passed to JS_NumberValue, which calls mozilla::NumberIsInt32 to detect Int32 representable values. JS::NumberValue performs this Int32-valued check with simple branches avoiding to call mozilla::NumberIsInt32.

Ah yes. A workaround for the std::isnan issue could also improve DOM bindings performance a bit on Linux. (Assuming this affects browser builds the same as shell builds; I haven't checked.)

(In reply to André Bargull [:anba] from comment #5)

FTR: Using d != d will probably require disabling the "NaNExprChecker.h" clang-plugin.

Good point. Another option is __builtin_isnan. (Maybe it's time to remove NaNExprChecker, I think it was added for buggy MSVC versions back then, e.g. bug 977538...)

Comment 9

[Jan de Mooij [:jandem]]

Attachment: (secure)

This avoids a much slower out-of-line call for std::isnan on Linux.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: (secure)

Clang is smart enough to optimize away the NaN check for setNumber(uint32_t) and
setNumber(uint64_t).

JS::CanonicalizedDoubleValue now forwards to JS::DoubleValue. We should remove it,
but this can happen in a follow-up bug. Similarly, JS_NumberValue just calls
JS::NumberValue for now.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: (secure)

Comment 12

[Jan de Mooij [:jandem]]

Attachment: (secure)

These behave like setDouble and setNumber before part 2.

Comment 13

[Jan de Mooij [:jandem]]

I've put up patches for this, but I haven't done any Try pushes yet (including perf comparisons) because I'm waiting for bug 2050657 and bug 2050990 to land first since they involves sandbox issues.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/84f64e3e321b
https://hg.mozilla.org/integration/autoland/rev/4d6dd15febb8
part 1 - Use __builtin_isnan in JS::CanonicalizeNaN to ensure the check is inlined. r=mgaudet
https://github.com/mozilla-firefox/firefox/commit/67ed94b1c4cf
https://hg.mozilla.org/integration/autoland/rev/4097f5d4b08b
part 2 - Always canonicalize NaNs for doubles stored in JS::Value. r=mgaudet
https://github.com/mozilla-firefox/firefox/commit/d42e3f11570c
https://hg.mozilla.org/integration/autoland/rev/97a0e5cf651f
part 3 - Remove redundant JS::CanonicalizeNaN calls. r=mgaudet
https://github.com/mozilla-firefox/firefox/commit/b6826718078f
https://hg.mozilla.org/integration/autoland/rev/88c178c93adf
part 4 - Add set{Double,Number}AssumeCanonicalNaN and call them in the interpreter and Math builtins. r=jonco

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4d6dd15febb8
https://hg.mozilla.org/mozilla-central/rev/4097f5d4b08b
https://hg.mozilla.org/mozilla-central/rev/97a0e5cf651f
https://hg.mozilla.org/mozilla-central/rev/88c178c93adf

Comment 17

[Jan de Mooij [:jandem]]

We should uplift this to ESR153 to avoid a footgun when people uplift patches (safe on 154+ but vulnerable on ESR153).

Comment 18

[Jan de Mooij [:jandem]]

Attachment: (secure)

This avoids a much slower out-of-line call for std::isnan on Linux.

Original Revision: https://phabricator.services.mozilla.com/D309383

Comment 19

[Jan de Mooij [:jandem]]

Attachment: (secure)

Clang is smart enough to optimize away the NaN check for setNumber(uint32_t) and
setNumber(uint64_t).

JS::CanonicalizedDoubleValue now forwards to JS::DoubleValue. We should remove it,
but this can happen in a follow-up bug. Similarly, JS_NumberValue just calls
JS::NumberValue for now.

Original Revision: https://phabricator.services.mozilla.com/D309384

Comment 20

[Jan de Mooij [:jandem]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D309385

Comment 21

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Security bugs, especially a risk for patches that will be uplifted to ESR153
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: Low-Medium. Multiple patches but they're pretty small and code is exercised by lots of tests.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 22

[Jan de Mooij [:jandem]]

Attachment: (secure)

These behave like setDouble and setNumber before part 2.

Original Revision: https://phabricator.services.mozilla.com/D309386

Comment 23

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/b6be0ea50011
https://hg.mozilla.org/releases/mozilla-beta/rev/c95c1260b54f
https://github.com/mozilla-firefox/firefox/commit/bd4a4b3d12b7
https://hg.mozilla.org/releases/mozilla-beta/rev/365ada2dfea7
https://github.com/mozilla-firefox/firefox/commit/4edea712adef
https://hg.mozilla.org/releases/mozilla-beta/rev/359b69690afc
https://github.com/mozilla-firefox/firefox/commit/9a5ad28957c1
https://hg.mozilla.org/releases/mozilla-beta/rev/e8005dfb5d27