Closed
Bug 1484905
Opened 6 years ago
Closed 6 years ago
Assertion failure: Length should be greater than 0., at js/src/jit/MacroAssembler.cpp:2031
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla64
People
(Reporter: gkw, Assigned: jandem)
References
Details
(6 keywords, Whiteboard: [jsbugmon:][adv-main63+][adv-esr60.3+])
Attachments
(2 files)
660 bytes,
text/plain
|
Details | |
46 bytes,
text/x-phabricator-request
|
arai
:
review+
pascalc
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
Details | Review |
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.
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
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)
Comment 3•6 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
Reporter | ||
Comment 4•6 years ago
|
||
(Sorry, there are tabs in the testcase if you copy it, feel free to change back to spaces)
Comment 5•6 years ago
|
||
(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)
Reporter | ||
Comment 6•6 years ago
|
||
(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)
Updated•6 years ago
|
Keywords: sec-moderate
Updated•6 years ago
|
Keywords: csectype-bounds
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
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+
Assignee | ||
Comment 10•6 years ago
|
||
Pushed this because I think the current sec-moderate rating is reasonable. https://hg.mozilla.org/integration/mozilla-inbound/rev/5c25f4ef0c29b4fab4b7ee53c8fa9f402874b7aa
Comment 11•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5c25f4ef0c29
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 12•6 years ago
|
||
Is this something we should consider backporting (affected code landed in bug 1263490, AFAICT)?
status-firefox62:
--- → wontfix
status-firefox-esr60:
--- → affected
Flags: needinfo?(jdemooij)
Flags: in-testsuite+
Assignee | ||
Comment 13•6 years ago
|
||
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 14•6 years ago
|
||
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+
Comment 15•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e43b9c1507d221650d7155607dec87b18752e5b5
Comment 16•6 years ago
|
||
(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 17•6 years ago
|
||
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+
Comment 18•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/35c26bc231df
Updated•6 years ago
|
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main63+][adv-esr60.3+]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•