Last Comment Bug 987935 - OOM: inlineScriptedCall() forgets to check TypeSet::clone() return
: OOM: inlineScriptedCall() forgets to check TypeSet::clone() return
Status: RESOLVED FIXED
[adv-main31-]
: sec-other
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla31
Assigned To: Sean Stangl [:sstangl]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 912928
  Show dependency treegraph
 
Reported: 2014-03-25 12:40 PDT by Sean Stangl [:sstangl]
Modified: 2016-06-04 15:33 PDT (History)
7 users (show)
kjozwiak: qe‑verify-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
wontfix


Attachments
patch (1.16 KB, patch)
2014-03-25 12:41 PDT, Sean Stangl [:sstangl]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2014-03-25 12:40:56 PDT
Simple thinko. Testcase from decoder (js__jit__MTypeBarrier__MTypeBarrier.txt). Probably not exploitable.
Comment 1 User image Sean Stangl [:sstangl] 2014-03-25 12:41:38 PDT
Created attachment 8396642 [details] [diff] [review]
patch
Comment 2 User image Andrew McCreight [:mccr8] 2014-03-25 13:38:22 PDT
Marking sec-other per comment 0. Feel free to change.
Comment 3 User image Nicolas B. Pierron [:nbp] 2014-03-27 03:45:51 PDT
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 5 User image Phil Ringnalda (:philor) 2014-03-29 09:13:05 PDT
https://hg.mozilla.org/mozilla-central/rev/2cb79b338122
Comment 6 User image Al Billings [:abillings] 2014-04-11 13:23:24 PDT
Do we need to worry about this on older releases at all?
Comment 7 User image Lukas Blakk [:lsblakk] use ?needinfo 2014-07-02 17:44:10 PDT
Do we need to uplift this to ESR24?
Comment 8 User image Al Billings [:abillings] 2014-07-03 10:37:27 PDT
(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 User image Kamil Jozwiak [:kjozwiak] 2014-07-09 08:02:39 PDT
Sean, is there anything QA can do here to test this? Comment #0 is pretty vague.
Comment 10 User image Sean Stangl [:sstangl] 2014-09-10 13:17:17 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.