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

RESOLVED FIXED in Firefox 31

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: sstangl, Assigned: sstangl)

Tracking

(Blocks: 1 bug, {sec-other})

unspecified
mozilla31
x86_64
Linux
sec-other
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox31 fixed, firefox-esr24 wontfix)

Details

(Whiteboard: [adv-main31-])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Simple thinko. Testcase from decoder (js__jit__MTypeBarrier__MTypeBarrier.txt). Probably not exploitable.
(Assignee)

Comment 1

3 years ago
Created attachment 8396642 [details] [diff] [review]
patch
Attachment #8396642 - Flags: review?(nicolas.b.pierron)
Keywords: sec-other
Marking sec-other per comment 0. Feel free to change.
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.
Attachment #8396642 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb79b338122
https://hg.mozilla.org/mozilla-central/rev/2cb79b338122
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox31: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Do we need to worry about this on older releases at all?
Do we need to uplift this to ESR24?
status-firefox-esr24: --- → ?
Flags: needinfo?(sstangl)
(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. :-)
status-firefox-esr24: ? → wontfix
Flags: needinfo?(sstangl)
Assignee: nobody → sstangl
Sean, is there anything QA can do here to test this? Comment #0 is pretty vague.
Flags: needinfo?(sstangl)
Whiteboard: [adv-main31-]
(Assignee)

Comment 10

3 years ago
(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.
Flags: needinfo?(sstangl)
Flags: qe-verify-

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.