Closed
Bug 1155474
Opened 9 years ago
Closed 9 years ago
Crash [@ js::Shape::search] with heap-buffer-overflow
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox37 | --- | wontfix |
firefox38 | + | verified |
firefox38.0.5 | --- | fixed |
firefox39 | + | verified |
firefox40 | --- | verified |
firefox41 | --- | verified |
firefox-esr31 | --- | unaffected |
firefox-esr38 | --- | fixed |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: decoder, Assigned: shu)
References
Details
(5 keywords, Whiteboard: [jsbugmon:update][adv-main38+])
Crash Data
Attachments
(1 file)
1.41 KB,
patch
|
Waldo
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision de27ac2ab94f (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off): { for (var i = 0; i < 100; i++) arr += arr.foo; let arr = 1; } Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x00000000005e5cb7 in js::Shape::search (cx=cx@entry=0x7ffff698d0f0, start=0x7ffff69223f8, id=..., pentry=pentry@entry=0x7fffffffcca0, adding=adding@entry=false) at js/src/vm/Shape-inl.h:44 #0 0x00000000005e5cb7 in js::Shape::search (cx=cx@entry=0x7ffff698d0f0, start=0x7ffff69223f8, id=..., pentry=pentry@entry=0x7fffffffcca0, adding=adding@entry=false) at js/src/vm/Shape-inl.h:44 #1 0x00000000005c32b8 in lookup (id=..., cx=0x7ffff698d0f0, this=<optimized out>) at js/src/vm/NativeObject.cpp:224 #2 LookupOwnPropertyInline<(js::AllowGC)1> (donep=<synthetic pointer>, propp=..., id=..., obj=..., cx=0x7ffff698d0f0) at js/src/vm/NativeObject-inl.h:460 #3 NativeGetPropertyInline<(js::AllowGC)1> (vp=..., nameLookup=NotNameLookup, id=..., receiver=..., obj=..., cx=0x7ffff698d0f0) at js/src/vm/NativeObject.cpp:1831 #4 js::NativeGetProperty (cx=0x7ffff698d0f0, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:1875 #5 0x00000000008a6855 in GetProperty (vp=..., id=..., receiver=..., obj=..., cx=0x7ffff698d0f0, cx@entry=0x7fffffffcec0) at js/src/vm/NativeObject.h:1417 #6 MaybeCallMethod (cx=cx@entry=0x7ffff698d0f0, obj=obj@entry=..., id=..., id@entry=..., vp=vp@entry=...) at js/src/jsobj.cpp:3315 #7 0x00000000008ac7a4 in JS::OrdinaryToPrimitive (cx=0x7ffff698d0f0, obj=..., hint=JSTYPE_VOID, vp=...) at js/src/jsobj.cpp:3377 #8 0x000000000055d027 in ToPrimitive (vp=..., cx=0x7ffff698d0f0) at js/src/jsobjinlines.h:545 #9 AddOperation (res=..., rhs=..., lhs=..., cx=0x7ffff698d0f0) at js/src/vm/Interpreter.cpp:1537 #10 js::AddValues (cx=cx@entry=0x7ffff698d0f0, lhs=..., lhs@entry=..., rhs=..., rhs@entry=..., res=..., res@entry=...) at js/src/vm/Interpreter.cpp:4421 #11 0x00000000006b7954 in js::jit::DoBinaryArithFallback (cx=0x7ffff698d0f0, frame=0x7fffffffd118, stub_=0x7ffff69a0720, lhs=..., rhs=..., ret=...) at js/src/jit/BaselineIC.cpp:2549 #12 0x00007ffff7feebc4 in ?? () #13 0x00007ffff7fe8640 in ?? () #14 0x00007fffffffd0b8 in ?? () #15 0x00007ffff7fe8640 in ?? () #16 0xfff9000000000000 in ?? () #17 0x000000000171aba0 in js::jit::DoConcatStringsInfo () #18 0x00007ffff4d51d60 in ?? () [...] #28 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7ffff69223f8 140737330160632 rcx 0x7fffffffcca0 140737488342176 rdx 0x0 0 rsi 0x7ffff4d1cc10 140737300778000 rdi 0x7ffff698d0f0 140737330598128 rbp 0x7ffff4d1cc10 140737300778000 rsp 0x7fffffffcbe0 140737488341984 r8 0x0 0 r9 0x7ffff4d5e128 140737301045544 r10 0x7fffffffd0e0 140737488343264 r11 0xfff9000000000000 -1970324836974592 r12 0x7ffff698d108 140737330598152 r13 0x7fffffffcc60 140737488342112 r14 0x7fffffffcd90 140737488342416 r15 0x7ffff698d0f0 140737330598128 rip 0x5e5cb7 <js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool)+167> => 0x5e5cb7 <js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool)+167>: mov 0x20(%rax),%rdi 0x5e5cbb <js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool)+171>: callq 0x5bfed0 <js::ShapeTable::search(jsid, bool)> ASan trace: ================================================================= ==12534==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60800003a68d at pc 0xa94398 bp 0x7fff97efdb70 sp 0x7fff97efdb68 READ of size 1 at 0x60800003a68d thread T0 #0 0xa94397 in js::Shape::inDictionary() const js/src/vm/Shape.h:786 #1 0xa94397 in js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool) js/src/vm/Shape-inl.h:43 #2 0x9fe521 in js::NativeObject::lookup(js::ExclusiveContext*, jsid) js/src/vm/NativeObject.cpp:224:12 #3 0x9fe521 in bool js::LookupOwnPropertyInline<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::MutableHandleType, bool*) js/src/vm/NativeObject-inl.h:460 #4 0x9fe521 in _ZL23NativeGetPropertyInlineILN2js7AllowGCE1EEbP9JSContextNS0_11MaybeRootedIPNS0_12NativeObjectEXT_EE10HandleTypeENS4_IP8JSObjectXT_EE10HandleTypeENS4_I4jsidXT_EE10HandleTypeE12IsNameLoo kupNS4_IN2JS5ValueEXT_EE17MutableHandleTypeE js/src/vm/NativeObject.cpp:1831 #5 0x9fe521 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/vm/NativeObject.cpp:1875 #6 0x151a2f0 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/vm/Nat iveObject.h:1417 #7 0x151a2f0 in MaybeCallMethod(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:3315 #8 0x1518424 in JS::OrdinaryToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:3377 #9 0x15179a1 in js::ToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:3196 #10 0x946317 in js::ToPrimitive(JSContext*, JS::MutableHandle<JS::Value>) js/src/jsobjinlines.h:545 #11 0x946317 in AddOperation(JSContext*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:1537 #12 0x946317 in js::AddValues(JSContext*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:4421 #13 0xd607a1 in js::jit::DoBinaryArithFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICBinaryArith_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:2549 0x60800003a68d is located 19 bytes to the left of 96-byte region [0x60800003a6a0,0x60800003a700) allocated by thread T0 here: #0 0x4abd47 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74 #1 0x7acce4 in js_malloc(unsigned long) js/src/opt64asan/js/src/../../dist/include/js/Utility.h:119 #2 0x7acce4 in _ZL13js_pod_mallocIhEPT_m js/src/opt64asan/js/src/../../dist/include/js/Utility.h:274 #3 0x7acce4 in unsigned char* js::MallocProvider<js::ExclusiveContext>::pod_malloc<unsigned char>(unsigned long) js/src/vm/MallocProvider.h:62 #4 0x7acce4 in js::ScriptSource* js::MallocProvider<js::ExclusiveContext>::new_<js::ScriptSource>() js/src/vm/MallocProvider.h:161 #5 0x7acce4 in js::frontend::CreateScriptSourceObject(js::ExclusiveContext*, JS::ReadOnlyCompileOptions const&) js/src/frontend/BytecodeCompiler.cpp:181 #6 0x7ad99e in js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Handle<js::StaticEvalObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JSString*, unsigned int, js::SourceCompressionTask*) js/src/frontend/BytecodeCompiler.cpp:246 #7 0x1402e3b in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3792 #8 0x14033f2 in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3802 #9 0x14033f2 in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3817 #10 0x140364c in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, _IO_FILE*, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3828 SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/vm/Shape.h:786 js::Shape::inDictionary() const Marking s-s and sec-critical because ASan shows this crash as a heap-buffer-overflow:
Reporter | ||
Updated•9 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 1•9 years ago
|
||
JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "bad" changeset has the timestamp "20150416202025" and the hash "1a51b749299f". The "good" changeset has the timestamp "20150416203330" and the hash "66eee8b402fd". Likely fix window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1a51b749299f&tochange=66eee8b402fd
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #2) > Needinfo from shu based on comment 1. Am I reading autobisect right in that it thought it was fixed by one of those patches?
Assignee | ||
Comment 4•9 years ago
|
||
I have no idea from the stacks if either of my patches were proper fixes for this bug.
Flags: needinfo?(shu)
Reporter | ||
Comment 5•9 years ago
|
||
That's indeed a good question, a fix bisection was not requested. gkw, can you maybe look manually why autobisect produced the wrong result here? Thanks!
Flags: needinfo?(gary)
(In reply to Christian Holler (:decoder) from comment #5) > That's indeed a good question, a fix bisection was not requested. gkw, can > you maybe look manually why autobisect produced the wrong result here? > Thanks! I ran autoBisect on downloaded binaries here on my computer and it shows latest binaries to still be showing the problem. Not sure why that happened for JSBugMon. Next, I ran it on compiled binaries and it shows: Due to skipped revisions, the first bad revision could be any of: changeset: https://hg.mozilla.org/mozilla-central/rev/03242a11d044 user: Shu-yu Guo date: Mon Sep 15 16:30:45 2014 -0700 summary: Bug 1001090 - Part 2a: Compile new let opcodes in Baseline. (r=jandem) changeset: https://hg.mozilla.org/mozilla-central/rev/8114e77c253e user: Shu-yu Guo date: Mon Sep 15 16:30:46 2014 -0700 summary: Bug 1001090 - Part 2b: Fix unwinding all scopes to not use pc. (r=jimb) changeset: https://hg.mozilla.org/mozilla-central/rev/31714af41f2c user: Shu-yu Guo date: Mon Sep 15 16:30:46 2014 -0700 summary: Bug 1001090 - Part 3: Compile new let opcodes in Ion. (r=jandem) Shu-yu, is bug 1001090 a likely regressor?
Blocks: 1001090
Flags: needinfo?(gary) → needinfo?(shu)
Assignee | ||
Comment 7•9 years ago
|
||
What's going on here is Ion compiled a lexical check as MThrowUninitializedLexical, since it saw that the input is for sure not initialized. MThrowUninitializedLexical has no inputs, so then Ion optimized out the JS_UNINITIALIZED_LEXICAL magic in favor of a JS_OPTIMIZED_OUT magic. When we bailed out, the BaselineFrame now has a wrong magic value for an uninitialized lexical.
Attachment #8596145 -
Flags: review?(jdemooij)
Assignee | ||
Comment 8•9 years ago
|
||
I think this will need backport to everything.
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Flags: needinfo?(shu)
Updated•9 years ago
|
Assignee: nobody → shu
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → affected
Comment 9•9 years ago
|
||
Comment on attachment 8596145 [details] [diff] [review] Consider the input to MThrowUninitializedLexical implicitly used. Review of attachment 8596145 [details] [diff] [review]: ----------------------------------------------------------------- Stealing upon request.
Attachment #8596145 -
Flags: review?(jdemooij) → review+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/81ed40faade8 This landed without sec-approval (and a test)? Please request sec-approval and Aurora/Release uplift approval ASAP.
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38.0.5:
--- → affected
Flags: needinfo?(shu)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10) > https://hg.mozilla.org/mozilla-central/rev/81ed40faade8 > > This landed without sec-approval (and a test)? Please request sec-approval > and Aurora/Release uplift approval ASAP. Argh sorry, I don't think I've ever remembered to request sec-approval in my life.
Flags: needinfo?(shu)
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8596145 [details] [diff] [review] Consider the input to MThrowUninitializedLexical implicitly used. [Security approval request comment] How easily could an exploit be constructed based on the patch? Fairly difficult as it requires a situation where the script executes in Ion before executing in Baseline/Interpreter. This generally isn't possible without special prefs/flags. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The test case itself is an exploit, only with --ion-eager. In normal content code it is not an exploit. Which older supported branches are affected by this flaw? All the way back to release. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Same patch can be backported. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Attachment #8596145 -
Flags: sec-approval?
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8596145 [details] [diff] [review] Consider the input to MThrowUninitializedLexical implicitly used. Approval Request Comment [Feature/regressing bug #]: 1001090 [User impact if declined]: Rare crashes when using lexicals in versioned JS (unavailable to unversioned content JS) [Describe test coverage new/current, TreeHerder]: on m-c [Risks and why]: Medium. An exploit could potentially be constructed from the patch. I don't see how right now, but there are cleverer people than I. [String/UUID change made/needed]: None
Attachment #8596145 -
Flags: approval-mozilla-release?
Attachment #8596145 -
Flags: approval-mozilla-beta?
Attachment #8596145 -
Flags: approval-mozilla-aurora?
Comment 14•9 years ago
|
||
> [Risks and why]: Medium. An exploit could potentially be constructed from the patch. I don't see how right now, but there are cleverer people than I.
The risk is the potential impact on the Firefox product itself. Not the security risk.
Could you evaluate it? Thanks
Updated•9 years ago
|
tracking-firefox38:
--- → +
tracking-firefox39:
--- → +
Comment 15•9 years ago
|
||
Comment on attachment 8596145 [details] [diff] [review] Consider the input to MThrowUninitializedLexical implicitly used. sec-approval+ for trunk.
Attachment #8596145 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #14) > > [Risks and why]: Medium. An exploit could potentially be constructed from the patch. I don't see how right now, but there are cleverer people than I. > The risk is the potential impact on the Firefox product itself. Not the > security risk. > Could you evaluate it? Thanks Sorry, what does impact on the product itself mean? Like could it exhibit buggy behavior? Web-breaking changes? That kind of thing?
Comment 17•9 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #16) > (In reply to Sylvestre Ledru [:sylvestre] from comment #14) > Sorry, what does impact on the product itself mean? Like could it exhibit > buggy behavior? Web-breaking changes? That kind of thing? Yes. For example: "high risk, this could break every javascript-based application"
Comment 18•9 years ago
|
||
Comment on attachment 8596145 [details] [diff] [review] Consider the input to MThrowUninitializedLexical implicitly used. Taking it as this is low impact and it should not land in the RC anyway.
Attachment #8596145 -
Flags: approval-mozilla-release?
Attachment #8596145 -
Flags: approval-mozilla-release+
Attachment #8596145 -
Flags: approval-mozilla-beta?
Attachment #8596145 -
Flags: approval-mozilla-aurora?
Attachment #8596145 -
Flags: approval-mozilla-aurora+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bc69f217d41 https://hg.mozilla.org/releases/mozilla-release/rev/daaa2c27b89f https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1063eb65195a
Comment 20•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19) > https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1063eb65195a Correction: https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d4135355e5bd
Updated•9 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main38+]
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
status-firefox41:
--- → verified
Reporter | ||
Comment 22•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx38 JSBugMon: This bug has been automatically verified fixed on Fx39 JSBugMon: This bug has been automatically verified fixed on Fx40
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
Updated•7 years ago
|
Keywords: csectype-bounds
You need to log in
before you can comment on or make changes to this bug.
Description
•