Closed
Bug 1264823
Opened 8 years ago
Closed 8 years ago
Assertion failure: val.isNull(), at js/src/builtin/MapObject.cpp:205 with OOM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | verified |
firefox-esr45 | --- | unaffected |
People
(Reporter: decoder, Assigned: arai)
References
(Blocks 1 open bug)
Details
(5 keywords, Whiteboard: [jsbugmon:update])
Attachments
(1 file, 1 obsolete file)
3.51 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision fb921246e2d6 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe): loadFile(""); loadFile(""); loadFile(` function lalala() {} new Map([[1, 2]]).forEach(lalala) `); function loadFile(lfVarx) oomTest(function() { eval(lfVarx) }) Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x084cf2e9 in js::MapIteratorObject::next (cx=cx@entry=0xf7a79020, mapIterator=mapIterator@entry=..., resultPairObj=resultPairObj@entry=...) at js/src/builtin/MapObject.cpp:205 #0 0x084cf2e9 in js::MapIteratorObject::next (cx=cx@entry=0xf7a79020, mapIterator=mapIterator@entry=..., resultPairObj=resultPairObj@entry=...) at js/src/builtin/MapObject.cpp:205 #1 0x0876bd48 in intrinsic_GetNextMapEntryForIterator (cx=0xf7a79020, argc=2, vp=0xffffa9d0) at js/src/vm/SelfHosting.cpp:748 #2 0xf7fd7516 in ?? () #3 0xf7aa27e0 in ?? () #4 0xf7fd7bda in ?? () #5 0xf7aa1798 in ?? () #6 0xf7fc8c5c in ?? () #7 0x0825cb0a in EnterBaseline (cx=0xf7aa27e0, cx@entry=0xf7a79020, data=...) at js/src/jit/BaselineJIT.cpp:150 [...] #25 0x085564b0 in JS_CallFunction (cx=cx@entry=0xf7a79020, obj=..., fun=fun@entry=..., args=..., rval=rval@entry=...) at js/src/jsapi.cpp:2865 #26 0x088663e6 in OOMTest (cx=0xf7a79020, argc=1, vp=0xf42270b0) at js/src/builtin/TestingFunctions.cpp:1310 #27 0x0871d8ea in js::CallJSNative (cx=0xf7a79020, native=0x88660e0 <OOMTest(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235 [...] #39 main (argc=3, argv=0xffffcd14, envp=0xffffcd24) at js/src/shell/js.cpp:7443 eax 0x0 0 ebx 0x98a7314 160068372 ecx 0xf7e3a88c -136075124 edx 0x0 0 esi 0xffffa978 -22152 edi 0xffffa8f4 -22284 ebp 0xffffa938 4294945080 esp 0xffffa8c0 4294944960 eip 0x84cf2e9 <js::MapIteratorObject::next(JSContext*, JS::Handle<js::MapIteratorObject*>, JS::Handle<js::ArrayObject*>)+1017> => 0x84cf2e9 <js::MapIteratorObject::next(JSContext*, JS::Handle<js::MapIteratorObject*>, JS::Handle<js::ArrayObject*>)+1017>: movl $0xcd,0x0 0x84cf2f3 <js::MapIteratorObject::next(JSContext*, JS::Handle<js::MapIteratorObject*>, JS::Handle<js::ArrayObject*>)+1027>: call 0x8108030 <abort()>
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•8 years ago
|
||
JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20160404040520" and the hash "ce36169cf980f3674fb729a5a528c589974b4bc6". The "bad" changeset has the timestamp "20160404045116" and the hash "5a184c73fcd3dd98c896fcd74ca1dd1379bcd0dd". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ce36169cf980f3674fb729a5a528c589974b4bc6&tochange=5a184c73fcd3dd98c896fcd74ca1dd1379bcd0dd
Comment 2•8 years ago
|
||
Regression window can't be trusted here.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,bisect]
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 3•8 years ago
|
||
JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20160321195240" and the hash "b718c3f4e82f8339537fcdaa644c1c42ba4eb831". The "bad" changeset has the timestamp "20160321201237" and the hash "a0c0ea60eaefc2f2907527e77fd71fe8b84bb7eb". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b718c3f4e82f8339537fcdaa644c1c42ba4eb831&tochange=a0c0ea60eaefc2f2907527e77fd71fe8b84bb7eb
Comment 4•8 years ago
|
||
Did a local bisection, which worked fine here. The first bad revision is: changeset: 289372:a3d994656b2b user: Tooru Fujisawa <arai_a@mac.com> date: Sat Mar 19 02:42:08 2016 +0900 summary: Bug 1248289 - Part 1: Inline _GetNextMapEntryForIterator intrinsic. r=jandem
Flags: needinfo?(arai.unmht)
Comment 5•8 years ago
|
||
Decoder, it seems that autobisect didn't work well with this test case, for some reason. After I've realized that it was x86 only, I could reproduce it easily, in a *non* intermittent way, and could bisect to a more plausible culprit. This needinfo is just a heads-up that something might be wrong in autobisect and that you might want to have a look :)
Flags: needinfo?(choller)
Comment 6•8 years ago
|
||
Arai says this is about a pre-write-barrier and it should be marked as security-sensitive, as a matter of fact.
Group: core-security
Assignee | ||
Comment 7•8 years ago
|
||
MapIteratorNext function can throw OOM exception *between* _GetNextMapEntryForIterator call and |mapIterationResultPair[0] = null;|. > function MapIteratorNext() { > ... > var done = _GetNextMapEntryForIterator(O, mapIterationResultPair); > if (!done) { > // Steps 10.b-c (omitted). > > // Step 6. > var itemKind = UnsafeGetInt32FromReservedSlot(this, ITERATOR_SLOT_ITEM_KIND); > > var result; > if (itemKind === ITEM_KIND_KEY) { > // Step 10.d.i. > result = mapIterationResultPair[0]; > } else if (itemKind === ITEM_KIND_VALUE) { > // Step 10.d.ii. > result = mapIterationResultPair[1]; > } else { > // Step 10.d.iii. > assert(itemKind === ITEM_KIND_KEY_AND_VALUE, itemKind); > result = [mapIterationResultPair[0], mapIterationResultPair[1]]; > } > > mapIterationResultPair[0] = null; > mapIterationResultPair[1] = null; > ... > } at least at this line (not sure if there is any other place): > result = [mapIterationResultPair[0], mapIterationResultPair[1]]; In that case, the elements of mapIterationResultPair keep previous value on next _GetNextMapEntryForIterator call. that means we should perform pre-barrier. So, we should either: A. explicitly initialize the elements of mapIterationResultPair just before _GetNextMapEntryForIterator B. initialize the elements of mapIterationResultPair as soon as _GetNextMapEntryForIterator returns false, before creating any object (maybe assigning them to temporary variables) Will compare the performance between them.
Assignee | ||
Comment 8•8 years ago
|
||
then, what could happen if we forgot to perform pre-write-barrier? something other than memory leak?
Flags: needinfo?(jdemooij)
Comment 9•8 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #8) > then, what could happen if we forgot to perform pre-write-barrier? > something other than memory leak? The write barrier basically ensures values are always marked when an incremental GC is in progress. If we skip the barrier we can likely have a user-after-free because we think something is dead and sweep it, while there's still a reference to it.
Flags: needinfo?(jdemooij)
Comment 10•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #9) > user-after-free Heh, s/user/use/ of course.
Assignee | ||
Comment 11•8 years ago
|
||
plan B seems to be less robust and risky. I'm still hitting the assertion failure even if I put the initialization before creating array, so there seems to other place that can throw OOM. So plan A should be better. Here's the performance comparison: code: var map = new Map(); for (var i=0; i<100; i++) map.set(i, i); function f(m) { var res; for (var i = 10000; i--;) { for (var v of m) { res = v[0]; } } return res; } var t = elapsed(); f(map); print(elapsed() - t); result: x64 before: 28193 [us] A: 29268 [us]
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8742437 -
Flags: review?(jdemooij)
Assignee | ||
Comment 12•8 years ago
|
||
forgot to post the result on x86. result: x86 before: 27713 [us] A: 28672 [us]
Updated•8 years ago
|
Group: core-security → javascript-core-security
Reporter | ||
Comment 13•8 years ago
|
||
Forwarding needinfo to gkw. Can you check why the bisect in comment 3 happened while the correct bisect is in comment 4? Thanks!
Flags: needinfo?(choller) → needinfo?(gary)
Comment 14•8 years ago
|
||
arai, what if we do masm.patchableCallPreBarrier(..address.., MIRType_Value) Right before we do each of those: masm.storeValue(keyAddress, Address(result, elementsOffset), temp); masm.storeValue(valueAddress, Address(result, elementsOffset + sizeof(Value)), temp); Does that also fix the problem? That might be a bit faster as well.
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 15•8 years ago
|
||
Thanks, will check the performance. but we don't yet hit the use-after-free or similar thing due to skipping pre-barrier, so I'm not sure whether it fixes or not.
Comment 16•8 years ago
|
||
Ah, I see what you mean. If we add the pre-barrier it's safe to remove the isNull asserts right? That makes sense.
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #16) > Ah, I see what you mean. If we add the pre-barrier it's safe to remove the > isNull asserts right? That makes sense. Yes, by adding the pre-barrier, we'll remove the assertion that fails in this bug. As the assertion is wrong for current situation, and that's not required if we perform pre-barrier.
Assignee | ||
Comment 18•8 years ago
|
||
Added patchableCallPreBarrier call both to the key/value element of result pair array. Performance result on x64 (same code as comment #11) before: 28364 previous patch: 29493 this patch: 28464
Attachment #8742437 -
Attachment is obsolete: true
Attachment #8742437 -
Flags: review?(jdemooij)
Flags: needinfo?(arai.unmht)
Attachment #8742938 -
Flags: review?(jdemooij)
(In reply to Christian Holler (:decoder) from comment #13) > Forwarding needinfo to gkw. Can you check why the bisect in comment 3 > happened while the correct bisect is in comment 4? Thanks! It might be intermittent on some revisions going back, it might not. The bisections in the earlier comments are using downloaded archive.mozilla.org builds, so they might be where they are intermittent. It's hard to say without logs.
Flags: needinfo?(gary)
Comment 20•8 years ago
|
||
Comment on attachment 8742938 [details] [diff] [review] Add pre-barrier to the elements of mapIterationResultPair. Review of attachment 8742938 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful, thanks.
Attachment #8742938 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Thank you for reviewing! This is nightly-only regression from bug 1248289 (Firefox 48), will land the patch shortly.
Blocks: 1248289
status-firefox47:
--- → unaffected
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdcf362a0234ad70425b14c4d58568d7bd66381b Bug 1264823 - Add pre-barrier to the elements of mapIterationResultPair. r=jandem
Comment 23•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cdcf362a0234
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 24•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
status-firefox-esr45:
--- → unaffected
Keywords: csectype-other,
regression
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•