Closed Bug 1231224 Opened 4 years ago Closed 4 years ago

Fix Vector append calls to check for OOM

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(13 files)

1.25 KB, patch
nbp
: review+
Details | Diff | Splinter Review
7.33 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
1.70 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
1.29 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
899 bytes, patch
h4writer
: review+
Details | Diff | Splinter Review
2.44 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
3.74 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.79 KB, patch
nbp
: review+
Details | Diff | Splinter Review
3.55 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
7.67 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.31 KB, patch
luke
: review+
Details | Diff | Splinter Review
13.18 KB, patch
luke
: review+
Details | Diff | Splinter Review
3.65 KB, patch
jonco
: review+
Details | Diff | Splinter Review
Marking Vector::append* MOZ_WARN_UNUSED_RESULT reveals a bunch of places that need fixing.
This avoids leaking a pool if m_smallPools.append() fails.
Attachment #8696808 - Flags: review?(nicolas.b.pierron)
Some MTableSwitch methods should handle OOM.
Attachment #8696809 - Flags: review?(hv1989)
Attached patch Part 3 - OdinSplinter Review
Attachment #8696810 - Flags: review?(benj)
Attachment #8696811 - Flags: review?(jwalden+bmo)
Here we can just use infallibleAppend, as we call reserve() before the loop.
Attachment #8696812 - Flags: review?(hv1989)
Some copy constructors where we can't easily propagate OOM. As OOM here seems unlikely, it seems OK to crash.
Attachment #8696813 - Flags: review?(bhackett1024)
Some places can't easily propagate OOM I think so I used AutoEnterOOMUnsafeRegion there.
Attachment #8696814 - Flags: review?(jcoppeard)
selectInliningTargets can just use infallibleAppend because it calls reserve first.

analyzeNewLoopTypes can return false on OOM.
Attachment #8696816 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8696810 - Flags: review?(benj) → review+
Attachment #8696811 - Flags: review?(jwalden+bmo) → review+
Attachment #8696814 - Flags: review?(jcoppeard) → review+
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
Attachment #8696809 - Flags: review?(hv1989) → review+
Attachment #8696812 - Flags: review?(hv1989) → review+
Attachment #8696813 - Flags: review?(bhackett1024) → review+
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.
Attachment #8696816 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8696808 - Flags: review?(nicolas.b.pierron) → review+
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
Attachment #8697306 - Flags: review?(bhackett1024)
Attachment #8697306 - Flags: review?(bhackett1024) → review+
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.
Keywords: leave-open
Depends on: 1233401
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.
Attachment #8704245 - Flags: review?(terrence)
This makes setProfilingEnabled fallible and reports OOM when resize() fails.
Attachment #8704286 - Flags: review?(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?
Attachment #8704286 - Flags: review?(luke) → review+
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.
Attachment #8704330 - Flags: review?(luke)
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.
Attachment #8704330 - Flags: review?(luke) → review+
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.
Attachment #8704533 - Flags: review?(jcoppeard)
Blocks: 1237201
Comment on attachment 8704533 [details] [diff] [review]
Part 13 - Fix Statistics::initialize

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

Great, thanks for fixing this.
Attachment #8704533 - Flags: review?(jcoppeard) → review+
Attachment #8704245 - Flags: review?(terrence) → review+
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.