1355135 - Assertion failure: !IsNaN(value) (The value should not be NaN), at /home/worker/workspace/build/src/layout/style/nsCSSScanner.cpp:953

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central asan-debug rev 20170410-b1364675bdf5.

Assertion failure: !IsNaN(value) (The value should not be NaN), at /home/worker/workspace/build/src/layout/style/nsCSSScanner.cpp:953

ASAN:DEADLYSIGNAL
=================================================================
==27109==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fb32bf49317 bp 0x7ffccc45b8f0 sp 0x7ffccc45b7a0 T0)
==27109==The signal is caused by a WRITE memory access.
==27109==Hint: address points to the zero page.
    #0 0x7fb32bf49316 in nsCSSScanner::ScanNumber(nsCSSToken&) /home/worker/workspace/build/src/layout/style/nsCSSScanner.cpp:824:5
    #1 0x7fb32bf4a029 in nsCSSScanner::Next(nsCSSToken&, nsCSSScannerExclude) /home/worker/workspace/build/src/layout/style/nsCSSScanner.cpp:1268:12
    #2 0x7fb32bf93d99 in (anonymous namespace)::CSSParserImpl::GetToken(bool) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:3119:20
    #3 0x7fb32bf97c8d in (anonymous namespace)::CSSParserImpl::GetNextTokenLocation(bool, unsigned int*, unsigned int*) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:3135:8
    #4 0x7fb32bf9ad57 in (anonymous namespace)::CSSParserImpl::ParseVariant(nsCSSValue&, unsigned int, nsCSSProps::KTableEntry const*) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:7682:8
    #5 0x7fb32bfb5fd5 in (anonymous namespace)::CSSParserImpl::ParseSingleValueProperty(nsCSSValue&, nsCSSPropertyID) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:11978:10
    #6 0x7fb32bfb3491 in (anonymous namespace)::CSSParserImpl::ParseProperty(nsCSSPropertyID) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:11435:11
    #7 0x7fb32bfb22a1 in (anonymous namespace)::CSSParserImpl::ParseDeclaration(mozilla::css::Declaration*, unsigned int, bool, bool*, (anonymous namespace)::CSSParserImpl::nsCSSContextType) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:7260:10
    #8 0x7fb32bfb18fe in (anonymous namespace)::CSSParserImpl::ParseDeclarationBlock(unsigned int, (anonymous namespace)::CSSParserImpl::nsCSSContextType) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:6664:10
    #9 0x7fb32bf9445d in (anonymous namespace)::CSSParserImpl::ParseRuleSet(void (*)(mozilla::css::Rule*, void*), void*, bool) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:5406:42
    #10 0x7fb32bf1d60b in (anonymous namespace)::CSSParserImpl::ParseRule(nsAString const&, nsIURI*, nsIURI*, nsIPrincipal*, mozilla::css::Rule**) /home/worker/workspace/build/src/layout/style/nsCSSParser.cpp:1888:7
    #11 0x7fb32be648f4 in mozilla::CSSStyleSheet::InsertRuleInternal(nsAString const&, unsigned int, mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/style/CSSStyleSheet.cpp:788:13
    #12 0x7fb3299dd35d in mozilla::dom::CSSStyleSheetBinding::insertRule(JSContext*, JS::Handle<JSObject*>, mozilla::StyleSheet*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/CSSStyleSheetBinding.cpp:175:25
    #13 0x7fb32a4e29b8 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2953:13
    #14 0x7fb32eaac021 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #15 0x7fb32eaabbcd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:455:16

Comment 1

[Daniel Holbert [:dholbert]]

Looks like hiro added this assertion in bug 1290994, in response to bug 1290994 comment 4.

hiro, mind taking a look?

Comment 2

[Daniel Holbert [:dholbert]]

The testcase has a style rule with
opacity:999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999e-485

The "9999...99" number -- disregarding the "e" exponent for now -- is 309 "9" characters, so it's 1 less than 10^309. (or something like that)

So, taking the "e" exponent into consideration now, we've effectively got 10^309 * 10^-485, i.e. [huge number] * [basically 0].  This seems roughly equivalent to infinity * 0, and that's presumably how we end up with NaN here.

It looks like over in bug 1290994, we were trying to avoid performing 0*infinity, but we only checked for 0 being raised to a huge exponent.  We did not check for the equivalent operation of infinity being raised to an extremely-tiny tiny exponent.

So: perhaps if we extend the "value == 0" check (inside of the "gotE" case) to also include value == positive-or-negative-infinity, that might solve this?  (I'm guessing this testcase gives us value=infinity at that point in the code, but I don't know for sure)

Comment 3

[Daniel Holbert [:dholbert]]

Also: while you're fixing this, it would be great if you could adjust the assertion message [and/or add a comment alongside the assertion] so that it's clearer.

Assertions like this one, of the form...
 MOZ_ASSERT(object.IsFoo(), "object should be foo")
...are a pet peeve of mine, because they're very hard to reason about.  When they fail, it requires some reverse-engineering to figure out:
 (1) why we care to have an assertion [i.e. how horrifically wrong will things go, if the assertion fails?]
 (2) what justification we have in asserting about the thing in the first place [i.e. how do we know the condition should hold?]

Ideally the assertion message and/or a nearby comment should hint at either or ideally both of these things.

Comment 5

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 6

[Daniel Bodea [:danibodea]]

I have attempted reproducing this assertion failure on Ubuntu 20 with build Release v85.0.1 asan debug and Nightly v87.0a1 asan debug using the test case in comment 0, but the assertion could not be reproduced.

Could this issue have been fixed in the meantime? (or have I tested incorrectly?)
Can you confirm this?

Comment 7

[Jason Kratzer [:jkratzer]]

(In reply to Bodea Daniel [:danibodea] from comment #6)

I have attempted reproducing this assertion failure on Ubuntu 20 with build Release v85.0.1 asan debug and Nightly v87.0a1 asan debug using the test case in comment 0, but the assertion could not be reproduced.

Could this issue have been fixed in the meantime? (or have I tested incorrectly?)
Can you confirm this?

I am unable to repeat this as well. I attempted to bisect the original testcase however, the oldest available build on taskcluster did not trigger the assertion above. I think we can safely close this as WFM.

Comment 8

[Daniel Holbert [:dholbert]]

Yeah, nsCSSScanner.cpp doesn't exist anymore (it was replaced with rust code in the Stylo project, a few years back).

Let's call this WORKSFORME (really, fixed-by-code-removal via stylo).