987935 - OOM: inlineScriptedCall() forgets to check TypeSet::clone() return

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Simple thinko. Testcase from decoder (js__jit__MTypeBarrier__MTypeBarrier.txt). Probably not exploitable.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: patch

Comment 2

[Andrew McCreight [:mccr8]]

Marking sec-other per comment 0. Feel free to change.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8396642 [details] [diff] [review]
patch

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

::: js/src/jit/IonBuilder.cpp
@@ +3919,5 @@
>          types::StackTypeSet *types = types::TypeScript::ThisTypes(calleeScript);
>          if (!types->unknown()) {
> +            types::TemporaryTypeSet *clonedTypes = types->clone(alloc_->lifoAlloc());
> +            if (!clonedTypes)
> +                return false;

The default behaviour after returning false is preventing any re-compilation, in such case we might want to recompile and not disable future compilations.

I think we want to define a function named
  bool IonBuilder::oom() {
    abortReason_ = AbortReason_Alloc
    return false;
  }

and use this function instead.  Otherwise we can open a follow-up bug later.

Comment 4

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb79b338122

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2cb79b338122

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Do we need to worry about this on older releases at all?

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Do we need to uplift this to ESR24?

Comment 8

[Al Billings [:abillings - ex-MoCo]]

(In reply to Lukas Blakk [:lsblakk] from comment #7)
> Do we need to uplift this to ESR24?

There is nothing to indicate that we need to here and non one has come forward to say otherwise. :-)

Comment 9

[Kamil Jozwiak [:kjozwiak]]

Sean, is there anything QA can do here to test this? Comment #0 is pretty vague.

Comment 10

[Sean Stangl [:sstangl]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #9)
> Sean, is there anything QA can do here to test this? Comment #0 is pretty
> vague.

The bug was a missing nullptr test. Doesn't need followup.