Assertion failure: Length should be greater than 0., at js/src/jit/MacroAssembler.cpp:2031

RESOLVED FIXED in Firefox -esr60

Status

()

defect
--
critical
RESOLVED FIXED
Last year
12 days ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks 2 bugs, 6 keywords)

Trunk
mozilla64
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr6063+ fixed, firefox62 wontfix, firefox63+ fixed, firefox64+ fixed)

Details

(Whiteboard: [jsbugmon:][adv-main63+][adv-esr60.3+])

Attachments

(2 attachments)

The following testcase crashes on mozilla-central revision 93d0e291f458 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager --ion-limit-script-size=off --ion-gvn=off):

for (var i = 0; i < 1; ++i) {
	"".replace(/x/, "").replace(/y/, "12");
}

Backtrace:

Thread 1 "js-dbg-64-dm-li" received signal SIGTRAP, Trace/breakpoint trap.
0x0000288c62f72183 in ?? ()
(gdb) bt
#0  0x0000288c62f72183 in ?? ()
#1  0x0000000000000030 in ?? ()
#2  0x0000288c62f70aea in ?? ()
#3  0x0000000000000000 in ?? ()
/snip

For detailed crash information, see attachment.

Not entirely sure if this is s-s. It points to length issues, with a SIGTRAP, yet requires gvn to be off, but setting it to be s-s as a start.
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/411fa809129a
user:        André Bargull
date:        Fri Aug 10 08:50:28 2018 -0700
summary:     Bug 1480819 - Part 6: Add helper methods to MacroAssembler to work with CharEncoding and reduce code duplication. r=mgaudet

Andre, is bug 1480819 a likely regressor?
Blocks: 1480819
Flags: needinfo?(andrebargull)
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
(Sorry, there are tabs in the testcase if you copy it, feel free to change back to spaces)
(In reply to Gary Kwong [:gkw] [:nth10sd] - in-n-out; clearing backlog from comment #2)
> Andre, is bug 1480819 a likely regressor?

Unless I'm misinterpreting the code, the assertion added in bug 1480819 did uncover an existing bug where we have an OOB read on a string. (The OOB read is restricted to a single character.)

|GetFirstDollarIndex| is only called when the string has at least two elements [1], but because MGetFirstDollarIndex is movable and has an empty alias-set [2], the call to |GetFirstDollarIndex| can be moved before the length check. At least this is what I guess happens here. I don't know the effects of "--ion-limit-script-size=off --ion-gvn=off", so I can't tell why these additional options are necessary to reproduce the assertion failure. 

[1] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/builtin/RegExp.js#271-272
[2] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/src/jit/MIR.h#7618,7627
Flags: needinfo?(andrebargull)
(In reply to André Bargull [:anba] from comment #5)
> Unless I'm misinterpreting the code, the assertion added in bug 1480819 did
> uncover an existing bug where we have an OOB read on a string. (The OOB read
> is restricted to a single character.)

Moving the needinfo? to Jan in this case.
Flags: needinfo?(jdemooij)
Another option here is to remove the length > 0 assumption from codegen, I can do that too if you prefer.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Comment on attachment 9006039 [details]
Bug 1484905 - Don't mark MGetFirstDollarIndex as movable. r=arai

Tooru Fujisawa [:arai] has approved the revision.
Attachment #9006039 - Flags: review+
Pushed this because I think the current sec-moderate rating is reasonable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c25f4ef0c29b4fab4b7ee53c8fa9f402874b7aa
https://hg.mozilla.org/mozilla-central/rev/5c25f4ef0c29
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Is this something we should consider backporting (affected code landed in bug 1263490, AFAICT)?
Flags: needinfo?(jdemooij)
Flags: in-testsuite+
Comment on attachment 9006039 [details]
Bug 1484905 - Don't mark MGetFirstDollarIndex as movable. r=arai

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1263490.
[User impact if declined]: Maybe crashes or a minor security issue.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: It just marks an instruction as non-movable.
[String changes made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #9006039 - Flags: approval-mozilla-esr60?
Attachment #9006039 - Flags: approval-mozilla-beta?
Comment on attachment 9006039 [details]
Bug 1484905 - Don't mark MGetFirstDollarIndex as movable. r=arai

Uplift approved for 63 beta 5
Attachment #9006039 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jan de Mooij [:jandem] from comment #13)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.
Marking as qe-verify- based on Jan's assessment and the bug's automated coverage.
Flags: qe-verify-
Comment on attachment 9006039 [details]
Bug 1484905 - Don't mark MGetFirstDollarIndex as movable. r=arai

Fixes a JS sec issue, approved for ESR 60.3.
Attachment #9006039 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.