Closed Bug 1238582 Opened 4 years ago Closed 4 years ago

Assertion failure: (strElements.resize(2 * len)), at js/src/jsarray.cpp:1762 with OOM

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 6020a4cb41a7 (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 --thread-count=2):

v = [2147483651];
[v[0], v[oomAfterAllocations(2)]].sort()



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0817e250 in SortLexicographically (len=1, vec=0xffffc170, cx=0xf7a7b020) at js/src/jsarray.cpp:1762
#0  0x0817e250 in SortLexicographically (len=1, vec=0xffffc170, cx=0xf7a7b020) at js/src/jsarray.cpp:1762
#1  js::array_sort (cx=0xf7a7b020, argc=0, vp=0xf570b060) at js/src/jsarray.cpp:1925
#2  0x086ccbaa in js::CallJSNative (cx=0xf7a7b020, native=0x817c190 <js::array_sort(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#14 main (argc=4, argv=0xffffce54, envp=0xffffce68) at js/src/shell/js.cpp:6918
eax	0x0	0
ebx	0x9838434	159614004
ecx	0xf7e3b88c	-136071028
edx	0x0	0
esi	0x1	1
edi	0x1	1
ebp	0xffffc298	4294951576
esp	0xffffc010	4294950928
eip	0x817e250 <js::array_sort(JSContext*, unsigned int, JS::Value*)+8384>
=> 0x817e250 <js::array_sort(JSContext*, unsigned int, JS::Value*)+8384>:	movl   $0x6e2,0x0
   0x817e25a <js::array_sort(JSContext*, unsigned int, JS::Value*)+8394>:	call   0x80fe920 <abort()>


The assertion violated indicates that we might start operating on an array of insufficient length for the following operations. Marking s-s until developers have confirmed that the following code still handles that situation properly.
In SortLexicographically, we first do:

    /* MergeSort uses the upper half as scratch space. */
    if (!strElements.reserve(2 * len))
        return false;

Followed by some more code. Then we do:

    JS_ALWAYS_TRUE(strElements.resize(2 * len));

The OOM testing machinery triggers an OOM here, but I think in practice this can't happen because we reserved the memory.

Maybe we could use infallibleGrowByUninitialized(len) here. I'm not sure if MergeSort requires initialized memory. If it does, we could add infallibleGrowBy to Vector.

The 2 * len is also a bit suspicious. I think it can't overflow because we have some code in array_sort:

#if JS_BITS_PER_WORD == 32
    if (size_t(len) > size_t(-1) / (2 * sizeof(Value))) {
    ...

And 2 * sizeof(Value) happens to be >= sizeof(StringifiedElement) and sizeof(NumericElement), but we should assert/document this better.
(In reply to Jan de Mooij [:jandem] from comment #1)
> And 2 * sizeof(Value) happens to be >= sizeof(StringifiedElement) and
> sizeof(NumericElement), but we should assert/document this better.

Hm nvm, this is bogus. len * 2 is fine though because of the check I mentioned, sizeof(StringifiedElement) is not relevant.
Attached patch bug1238582-sortSplinter Review
I think we can just resize the vector earlier.
Assignee: nobody → jcoppeard
Attachment #8706968 - Flags: review?(jdemooij)
Attachment #8706968 - Attachment is patch: true
Comment on attachment 8706968 [details] [diff] [review]
bug1238582-sort

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

That's much nicer and also more efficient, thanks!

It looks like SortNumerically needs a similar fix.

::: js/src/jsarray.cpp
@@ +1741,5 @@
>      Vector<StringifiedElement, 0, TempAllocPolicy> strElements(cx);
>  
>      /* MergeSort uses the upper half as scratch space. */
> +    if (!strElements.resize(2 * len)) {
> +        ReportOutOfMemory(cx);

The Vector uses TempAllocPolicy/cx, I think that will also report OOM.
Attachment #8706968 - Flags: review?(jdemooij) → review+
This looks like an OOM, what happens after the assert fires in a non-debug build? Or is this a fatal assert even in release builds?
Keywords: csectype-oom
It's not even an OOM -- it's a mistake when we *simulate* OOM, only.  Only debug builds will crash, and only if simulating OOM, in a case where the simulated OOM is injected in a case that will never OOM in reality.  IMO this bug can be opened as a case of over-zealous OOM-simulation machinery meaning a false-assert happens.
That said.  Currently we have this code:

Vector<T, N, AP>::growBy(size_t aIncr)
{
  MOZ_REENTRANCY_GUARD_ET_AL;
  if (aIncr > mCapacity - mLength) {
    if (MOZ_UNLIKELY(!growStorageBy(aIncr))) {
      return false;
    }
  } else if (aIncr + mLength > N) {
    if (!allocPolicy().checkSimulatedOOM()) {
      return false;
    }
  }

jandem, can you think of a reason we can't condition the latter condition there on |aIncr + mLength| respecting reserved-space, too?  That is,

Vector<T, N, AP>::growBy(size_t aIncr)
{
  MOZ_REENTRANCY_GUARD_ET_AL;
  if (aIncr > mCapacity - mLength) {
    if (MOZ_UNLIKELY(!growStorageBy(aIncr))) {
      return false;
    }
  } else if (aIncr + mLength > N && aIncr + mLength > mReserved) {
    if (!allocPolicy().checkSimulatedOOM()) {
      return false;
    }
  }

Seems like that would eliminate the simulated-OOM that'll never happen, in the case where we're growing within reserved space.  Right?
Flags: needinfo?(jdemooij)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> jandem, can you think of a reason we can't condition the latter condition
> there on |aIncr + mLength| respecting reserved-space, too?  That is,

I think that works, but mReserved is #ifdef DEBUG. Most of the OOM machinery is also debug-only though, so just adding that extra check in an #ifdef DEBUG block is fine I think.
Flags: needinfo?(jdemooij)
Unmarking s-s since this is debug only and related to our testing infrastructure.  It's not possible for this assertion to fail due to OOM in a release build.
Group: javascript-core-security
https://hg.mozilla.org/mozilla-central/rev/cc5eff30f13f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Attached patch oom.diffSplinter Review
Attachment #8712369 - Flags: review?(jwalden+bmo)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment on attachment 8712369 [details] [diff] [review]
oom.diff

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

::: mfbt/Vector.h
@@ +980,1 @@
>      if (!allocPolicy().checkSimulatedOOM()) {

I'd say move the bool stuffs into this branch, i.e.

  } else if (aIncr + mLength > N) {
    bool checkSimulatedOOM =
#ifdef DEBUG
      aIncr + mLength > mReserved
#else
       true
#endif
      ;
    if (checkSimulatedOOM && !allocPolicy().checkSimulatedOOM()) {
      return false;
    }
  }
Attachment #8712369 - Flags: review?(jwalden+bmo) → review+
You need to log in before you can comment on or make changes to this bug.