Closed
Bug 1219883
Opened 9 years ago
Closed 9 years ago
Crash [@ js::jit::GetIndexFromString] or Assertion failure: !constant(), at js/src/jit/RegisterSets.h:254
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox42 | --- | unaffected |
firefox43 | --- | unaffected |
firefox44 | + | verified |
firefox45 | + | verified |
firefox-esr38 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-v2.5 | --- | fixed |
b2g-v2.2r | --- | unaffected |
b2g-master | --- | fixed |
People
(Reporter: rforbes, Assigned: jandem)
References
Details
(5 keywords, Whiteboard: [jsbugmon:][b2g-adv-main2.5-])
Crash Data
Attachments
(2 files, 1 obsolete file)
5.46 KB,
patch
|
jandem
:
review+
ritu
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
621 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 7b6a80ca1f0d (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2): function test() { var arr = new Int8Array(400); var idx = "384"; arr[idx] = 9; assertEq(arr[idx], 9); test(); } test(); Backtrace: Program received signal SIGSEGV, Segmentation fault. js::jit::GetIndexFromString (str=0x0) at js/src/jit/VMFunctions.cpp:644 #0 js::jit::GetIndexFromString (str=0x0) at js/src/jit/VMFunctions.cpp:644 #1 0x00007ffff7ff2ee2 in ?? () #2 0x00007ffff7e7dbf0 in ?? () #3 0x00007ffffff99650 in ?? () #4 0x4022000000000000 in ?? () #5 0x0000000000000000 in ?? () rax 0x6379d0 6519248 rbx 0x7ffff7ff282d 140737354082349 rcx 0xfffc7ffff4624360 -985162613374112 rdx 0x0 0 rsi 0x7ffff4624360 140737293468512 rdi 0x0 0 rbp 0x7ffffff99818 140737487935512 rsp 0x7ffffff99620 140737487935008 r8 0x7ffffff99758 140737487935320 r9 0x7ffffff99660 140737487935072 r10 0x180 384 r11 0x7ffff7e7dbf0 140737352555504 r12 0x8 8 r13 0x7fffffffc6c0 140737488340672 r14 0x0 0 r15 0x7ffffff99818 140737487935512 rip 0x6379d4 <js::jit::GetIndexFromString(JSString*)+4> => 0x6379d4 <js::jit::GetIndexFromString(JSString*)+4>: mov (%rdi),%edx 0x6379d6 <js::jit::GetIndexFromString(JSString*)+6>: mov $0xffffffff,%eax Marked as security due to crash with random memory address.
Assignee | ||
Comment 1•9 years ago
|
||
The GetPropertyIC's TypedArray stub can accept string or int32 indexes. After bug 1209118, the string can be a constant so we need to handle that case. If it's int32 it must be in a register.
Comment 2•9 years ago
|
||
How bad of a security issue is this, Jan? Do we end up putting in random garbage in the case of a constant?
Comment 3•9 years ago
|
||
Comment on attachment 8680850 [details] [diff] [review] Patch Review of attachment 8680850 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +3969,3 @@ > } else { > MOZ_ASSERT(idval.isInt32()); > prefer a MOZ_ASSERT(!index.constant()) explicitly here, despite that assert being entailed in the index.reg() calls. It will make explicit the link between integer ids and using registers.
Attachment #8680850 -
Flags: review?(efaustbmo) → review+
Updated•9 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Comment 4•9 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Updated•9 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Assignee | ||
Comment 6•9 years ago
|
||
[Security approval request comment] * How easily could an exploit be constructed based on the patch? Not sure, not trivial probably. Maybe with some effort. * Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. * Which older supported branches are affected by this flaw? 44+. * If not all supported branches, which bug introduced the flaw? Bug 1209118. * Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply to aurora. * How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8680850 -
Attachment is obsolete: true
Attachment #8687939 -
Flags: sec-approval?
Attachment #8687939 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8687940 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
status-firefox42:
--- → unaffected
status-firefox43:
--- → unaffected
status-firefox45:
--- → affected
status-firefox-esr38:
--- → unaffected
Updated•9 years ago
|
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-v2.5:
--- → affected
status-b2g-master:
--- → affected
Comment 8•9 years ago
|
||
This needs a rating before we can determine if it needs sec-approval to land. Do you have any suggestions? Andrew asked about this in comment 2 as well.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Al Billings [:abillings] from comment #8) > This needs a rating before we can determine if it needs sec-approval to > land. Do you have any suggestions? Hm it's hard to say but let's go with sec-high. We'll generate bogus JIT code and that's always scary.
Flags: needinfo?(jdemooij)
Keywords: sec-high
Comment 10•9 years ago
|
||
Comment on attachment 8687939 [details] [diff] [review] Part 1 - Fix sec-approval+. We'll want it nominated for 44 as well.
Attachment #8687939 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
tracking-firefox44:
--- → +
tracking-firefox45:
--- → +
Assignee | ||
Comment 11•9 years ago
|
||
Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/5dcdd3c728fa
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8687939 [details] [diff] [review] Part 1 - Fix Approval Request Comment [Feature/regressing bug #]: Bug 1209118. [User impact if declined]: Crashes, correctness/security bugs. [Describe test coverage new/current, TreeHerder]: Fixes the testcase. [Risks and why]: Very low risk. [String/UUID change made/needed]: None.
Attachment #8687939 -
Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5dcdd3c728fa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8687939 [details] [diff] [review] Part 1 - Fix Taking it because it's a sec fix. Aurora44+
Attachment #8687939 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Updated•9 years ago
|
Comment 17•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx44
Updated•9 years ago
|
Group: javascript-core-security → core-security-release
Updated•9 years ago
|
Comment 18•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx45
Updated•9 years ago
|
Whiteboard: [jsbugmon:] → [jsbugmon:][b2g-adv-main2.5-]
Updated•8 years ago
|
Group: core-security-release
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b7831bde59d
You need to log in
before you can comment on or make changes to this bug.
Description
•