Closed Bug 1498177 Opened Last year Closed Last year

Assertion failure: lowLimit < highLimit, at js/src/gc/GC.cpp:2098

Categories

(Core :: JavaScript Engine, defect, P3, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
thunderbird_esr52 --- unaffected
thunderbird_esr60 --- unaffected
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- fixed

People

(Reporter: gkw, Assigned: pbone)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision e4220fa7a191 (build with --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

See attachment.

Backtrace:

#0  0x00005555db887034 in js::gc::ZoneHeapThreshold::computeZoneHeapGrowthFactorForHeapSize (lastBytes=1048576, tunables=..., state=...) at js/src/gc/GC.cpp:2098
#1  0x00005555db883e90 in js::gc::ZoneHeapThreshold::updateAfterGC (this=<optimized out>, lastBytes=1048576, gckind=GC_NORMAL, tunables=..., state=..., lock=...) at js/src/gc/GC.cpp:2134
#2  js::gc::GCRuntime::setParameter (this=0x7ff7f941c6d0, key=<optimized out>, value=<optimized out>, lock=...) at js/src/gc/GC.cpp:1478
#3  0x00005555db143f53 in GCParameter (cx=0x7ff7f9418000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:555
#4  0x00005555dad2b295 in CallJSNative (cx=0x7ff7f9418000, native=0x5555db143940 <GCParameter(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:461
#5  0x00005555dad1d031 in js::InternalCallOrConstruct (cx=0x7ff7f9418000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:553
/snip

For detailed crash information, see attachment.

Setting s-s because GC seems involved, as a start.
Attached file Testcase
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5aee88f016b0
user:        Paul Bone
date:        Thu Oct 04 19:13:20 2018 +1000
summary:     Bug 1496699 - Convert some doubles to floats r=sfink

Paul, is bug 1496699 a likely regressor?
Blocks: 1496699
Flags: needinfo?(pbone)
This might just lead to bad heuristics in GC rather than a security bug? Will wait for feedback from Paul before rating.
Hi :gkw, that's a very likely regression :-(  :dveditz, robably not a security problem, but I'll double check.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Flags: needinfo?(pbone)
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
It's also a fairly harmless bug, the fix should be easy.
Priority: P1 → P3
These values were compared as floats, where there was no sagnificant
difference and the assertion failed.
Attachment #9016572 - Flags: review?(sphink)
Opening up as per comment 5 and comment 6.
Group: javascript-core-security
Comment on attachment 9016572 [details] [diff] [review]
Bug 1498177 - Use size_t for integer values particularly for comparisons

Review of attachment 9016572 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/gc/bug-1498177.js
@@ +23098,5 @@
> +"js/src/tests/test262/annexB/built-ins/unescape/browser.js",
> +"js/src/tests/test262/annexB/built-ins/unescape/four.js",
> +"js/src/tests/test262/annexB/built-ins/unescape/name.js",
> +"js/src/tests/test262/annexB/built-ins/unescape/to-string-err-symbol.js",
> +"js/src/tests/test262/annexB/built-ins/unescape/four-ignore-non-hex.js";

Uh, I guess all of the above was from a fuzz bug? Please remove it, it's not needed for what this is testing.
Attachment #9016572 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #9)
> Comment on attachment 9016572 [details] [diff] [review]
> Bug 1498177 - Use size_t for integer values particularly for comparisons
> 
> Review of attachment 9016572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/gc/bug-1498177.js
> @@ +23098,5 @@
> > +"js/src/tests/test262/annexB/built-ins/unescape/browser.js",
> > +"js/src/tests/test262/annexB/built-ins/unescape/four.js",
> > +"js/src/tests/test262/annexB/built-ins/unescape/name.js",
> > +"js/src/tests/test262/annexB/built-ins/unescape/to-string-err-symbol.js",
> > +"js/src/tests/test262/annexB/built-ins/unescape/four-ignore-non-hex.js";
> 
> Uh, I guess all of the above was from a fuzz bug? Please remove it, it's not
> needed for what this is testing.

Yes, but it needs it to trigger the bug, the code with the assertion is only triggerd if the GC is in a high allocation state.  I can force that another way and have a smaller object here.
The bug is only triggered when the zone is at least 1MB big, below that size only the low threshold settings are ever used.  This test case is smaller.
Attachment #9016572 - Attachment is obsolete: true
Attachment #9017061 - Flags: review?(sphink)
(In reply to Paul Bone [:pbone] from comment #10)
> (In reply to Steve Fink [:sfink] [:s:] from comment #9)
> > Comment on attachment 9016572 [details] [diff] [review]
> > Bug 1498177 - Use size_t for integer values particularly for comparisons
> > 
> > Review of attachment 9016572 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit-test/tests/gc/bug-1498177.js
> > @@ +23098,5 @@
> > > +"js/src/tests/test262/annexB/built-ins/unescape/browser.js",
> > > +"js/src/tests/test262/annexB/built-ins/unescape/four.js",
> > > +"js/src/tests/test262/annexB/built-ins/unescape/name.js",
> > > +"js/src/tests/test262/annexB/built-ins/unescape/to-string-err-symbol.js",
> > > +"js/src/tests/test262/annexB/built-ins/unescape/four-ignore-non-hex.js";
> > 
> > Uh, I guess all of the above was from a fuzz bug? Please remove it, it's not
> > needed for what this is testing.
> 
> Yes, but it needs it to trigger the bug, the code with the assertion is only
> triggerd if the GC is in a high allocation state.  I can force that another
> way and have a smaller object here.

Oh! That's fair, I didn't realize that. I was wrong, it *is* needed, then!
Comment on attachment 9017061 [details] [diff] [review]
Bug 1498177 - Use size_t for integer values particularly for comparisons r=sfink

Review of attachment 9017061 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that's easier to read. You might have been able to get away with just `for (...) Object.create(null)`, but I'm not sure. This is fine.
Attachment #9017061 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #14)
> Comment on attachment 9017061 [details] [diff] [review]
> Bug 1498177 - Use size_t for integer values particularly for comparisons
> r=sfink
> 
> Review of attachment 9017061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, that's easier to read. You might have been able to get away with
> just `for (...) Object.create(null)`, but I'm not sure. This is fine.

Yeah, I wasn't sure what kinds of objects were going to get packed into the pointer-word.
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f69bad9ab0
Use size_t for integer values particularly for comparisons r=sfink
https://hg.mozilla.org/mozilla-central/rev/e1f69bad9ab0
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.