1264823 - Assertion failure: val.isNull(), at js/src/builtin/MapObject.cpp:205 with OOM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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()>

Comment 1

[Fuzzing Team]

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

[Benjamin Bouvier [:bbouvier] (inactive)]

Regression window can't be trusted here.

Comment 3

[Fuzzing Team]

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

[Benjamin Bouvier [:bbouvier] (inactive)]

Did a local bisection, which worked fine here.

The first bad revision is:
changeset:   289372:a3d994656b2b
user:        Tooru Fujisawa <[e-mail]>
date:        Sat Mar 19 02:42:08 2016 +0900
summary:     Bug 1248289 - Part 1: Inline _GetNextMapEntryForIterator intrinsic. r=jandem

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

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 :)

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Arai says this is about a pre-write-barrier and it should be marked as security-sensitive, as a matter of fact.

Comment 7

[Tooru Fujisawa [:arai]]

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.

Comment 8

[Tooru Fujisawa [:arai]]

then, what could happen if we forgot to perform pre-write-barrier?
something other than memory leak?

Comment 9

[Jan de Mooij [:jandem]]

(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.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> user-after-free

Heh, s/user/use/ of course.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Make sure the element of mapIterationResultPair is set to null before calling _GetNextMapEntryForIterator.

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]

Comment 12

[Tooru Fujisawa [:arai]]

forgot to post the result on x86.

result:
  x86
    before: 27713 [us]
    A:      28672 [us]

Comment 13

[Christian Holler (:decoder)]

Forwarding needinfo to gkw. Can you check why the bisect in comment 3 happened while the correct bisect is in comment 4? Thanks!

Comment 14

[Jan de Mooij [:jandem]]

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.

Comment 15

[Tooru Fujisawa [:arai]]

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

[Jan de Mooij [:jandem]]

Ah, I see what you mean. If we add the pre-barrier it's safe to remove the isNull asserts right? That makes sense.

Comment 17

[Tooru Fujisawa [:arai]]

(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.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Add pre-barrier to the elements of mapIterationResultPair.

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

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8742938 [details] [diff] [review]
Add pre-barrier to the elements of mapIterationResultPair.

Review of attachment 8742938 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, thanks.

Comment 21

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

This is nightly-only regression from bug 1248289 (Firefox 48),
will land the patch shortly.

Comment 22

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdcf362a0234ad70425b14c4d58568d7bd66381b
Bug 1264823 - Add pre-barrier to the elements of mapIterationResultPair. r=jandem

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdcf362a0234

Comment 24

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.