1231224 - Fix Vector append calls to check for OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Marking Vector::append* MOZ_WARN_UNUSED_RESULT reveals a bunch of places that need fixing.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - ExecutableAllocator

This avoids leaking a pool if m_smallPools.append() fails.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - MTableSwitch

Some MTableSwitch methods should handle OOM.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Odin

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - TokenStream::getDirective

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Debugger::setupTraceLogger

Here we can just use infallibleAppend, as we call reserve() before the loop.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Ion copy constructors

Some copy constructors where we can't easily propagate OOM. As OOM here seems unlikely, it seems OK to crash.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 7 - More fixes

Some places can't easily propagate OOM I think so I used AutoEnterOOMUnsafeRegion there.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Some IonBuilder fixes

selectInliningTargets can just use infallibleAppend because it calls reserve first.

analyzeNewLoopTypes can return false on OOM.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8696810 [details] [diff] [review]
Part 3 - Odin

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

Indeed. We could also use a fallible resize call above the first loop and use infallibleAppend then, but that should not matter too much.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8696809 [details] [diff] [review]
Part 2 - MTableSwitch

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

Good find! Why didn't any fuzzer find this yet?

r+ with the comments addressed

::: js/src/jit/MIR.h
@@ +2543,1 @@
>      }

Can we still return the successor id? E.g using a outparam to return this info?
That way the inner workings are only visible here and not in the callers...

@@ +2584,3 @@
>          MOZ_ASSERT(successors_.empty());
> +        return successors_.append(block);
> +    }

Ditto

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8696816 [details] [diff] [review]
Part 8 - Some IonBuilder fixes

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

::: js/src/jit/IonBuilder.cpp
@@ +5396,5 @@
>              // Non-function targets are not supported by polymorphic inlining.
>              inlineable = false;
>          }
>  
> +        choiceSet.infallibleAppend(inlineable);

Note to self: This is fine as we reserve enough space before.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8696808 [details] [diff] [review]
Part 1 - ExecutableAllocator

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

Ok, I was not sure at the beginning, but the ExecutableAllocator has 2 mode of allocations, either we have a large allocation in which case we allocate the correct size for the exepected allocation, or we have a small allocation, in which case we bump it to a minimal-largeAllocSize and we instead it in the list of available pages where we can store smaller allocations.

Thus, this code is fine, as long as the original data is not invalidated/reclaimed on OOM.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 9 - StackSlotAllocator

A bunch of changes and clean up to StackSlotAllocator:

* freeSlot is unused (since LSRA was removed I think) so I removed it. BT has a different mechanism for reusing stack slots.
* As a result, quadSlots is never appended to so I removed it as well.
* I added a (void) cast to the remaining append() calls

Comment 14

[Jan de Mooij [:jandem]]

Pushed parts 1-8.

(In reply to Hannes Verschore [:h4writer] from comment #10)
> Can we still return the successor id? E.g using a outparam to return this
> info?

OK, done.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89a198f044bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a80b9fd6fd19
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe63a5b34d53
https://hg.mozilla.org/integration/mozilla-inbound/rev/96873f172d31
https://hg.mozilla.org/integration/mozilla-inbound/rev/5000824d37a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/136542de8e2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1e59872646
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b88627500d

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89a198f044bf
https://hg.mozilla.org/mozilla-central/rev/a80b9fd6fd19
https://hg.mozilla.org/mozilla-central/rev/fe63a5b34d53
https://hg.mozilla.org/mozilla-central/rev/96873f172d31
https://hg.mozilla.org/mozilla-central/rev/5000824d37a7
https://hg.mozilla.org/mozilla-central/rev/136542de8e2e
https://hg.mozilla.org/mozilla-central/rev/5a1e59872646
https://hg.mozilla.org/mozilla-central/rev/43b88627500d

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Part 10 - More fixes

This fixes more places, including some tests and debugging code.

I also added a shrinkTo method to Vector. Some places currently call resize(), which is fallible, and adding a MOZ_ALWAYS_TRUE to those callers is pretty verbose. shrinkBy is a bit awkward to call manually.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 11 - Module::setProfilingEnabled

This makes setProfilingEnabled fallible and reports OOM when resize() fails.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8704286 [details] [diff] [review]
Part 11 - Module::setProfilingEnabled

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

Thanks!

::: js/src/asmjs/WasmModule.cpp
@@ +663,5 @@
>                  continue;
>              unsigned lineno = codeRange.funcLineNumber();
>              const char* name = funcNames_[codeRange.funcNameIndex()].get();
>              funcLabels_[codeRange.funcNameIndex()] =
>                  UniqueChars(JS_smprintf("%s (%s:%u)", name, filename_.get(), lineno));

Could you also check and ReportOutOfMemory() for this one too?

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66ce05465c3

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 12 - Fix warnings in irregexp code

Irregexp uses an infallible Vector policy, but the compiler will still complain when the fallible Vector methods are marked MOZ_WARN_UNUSED_RESULTS.

This patch adds an InfallibleVector class and uses it instead.

This class can't inherit from Vector because Vector is 'final' atm. Maybe we could change that, but having this class *contain* a Vector seems nice too: we can enforce no fallible methods are called on these InfallibleVectors.

Comment 22

[Jan de Mooij [:jandem]]

With these patches, there are only ~3 warnings left when building the shell and marking all fallible Vector methods MOZ_WARN_UNUSED_RESULT.

However, I just built the browser and there are many more in js/src/ctypes. Vector is also becoming popular outside js/src, so there are warnings all over Gecko. Pretty annoying, but worth fixing.

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 13 - Fix Statistics::initialize

I just made Statistics::initialize return bool instead of void, so we can propagate OOM.

For some of these append calls we could use MOZ_ALWAYS_TRUE or reserve maybe, but this code is only called once per process so I didn't bother.

Comment 24

[Jon Coppeard (:jonco)]

Comment on attachment 8704533 [details] [diff] [review]
Part 13 - Fix Statistics::initialize

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

Great, thanks for fixing this.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80a318392bb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c2db363fa9
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6334c0964f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4dec7c64eba

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80a318392bb0
https://hg.mozilla.org/mozilla-central/rev/a1c2db363fa9
https://hg.mozilla.org/mozilla-central/rev/6d6334c0964f
https://hg.mozilla.org/mozilla-central/rev/d4dec7c64eba