Fix some bogus OOM-handling in JSScript methods

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

({sec-audit})

16 Branch
mozilla18
sec-audit
Points:
---

Firefox Tracking Flags

(firefox-esr10 unaffected, firefox-esr17 wontfix, b2g18 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
While looking at bug 787887 I found some related problems with OOM-handling.
I don't think they explain the crash in bug 787887 (which is a 
NULL-dereference), but they're worth fixing anyway.  I'm marking this
security-sensitive just to be safe.

- In JSScript::ensureHasDebugScript() we would erroneously delete
  compartment()->debugScriptMap if the table insertion failed, despite the 
  fact that it is a valid HashTable pointer.  This would leave
  compartment()->debugScriptMap as a dangling pointer.

- The same problem occurred in JSScript::initScriptCounts().  
  
- In initScriptCounts() we were also erroneously deleting |cursor| instead of
  |base| in that same OOM path.  I.e. it's a bogus delete that would 
  probably cause a crash if it was hit.
  
You'll need to look at the original code to understand the patches;  eight 
lines isn't enough context.
(Assignee)

Comment 1

5 years ago
Created attachment 659978 [details] [diff] [review]
Slightly improve some JSScript OOM handling.

While looking at bug 787887 I found some related problems with OOM-handling.
I don't think they explain the crash in bug 787887 (which is a 
NULL dereference), but they're worth fixing anyway.  Marking
security-sensitive just to be safe.

- In JSScript::ensureHasDebugScript() we would erroneously delete
  compartment()->debugScriptMap if the table insertion failed, despite the 
  fact that it is a valid HashTable pointer.  This would leave
  compartment()->debugScriptMap as a dangling pointer.

- The same problem occurred in JSScript::initScriptCounts().  
  
- In initScriptCounts() we were also erroneously deleting |cursor| instead of
  |base| in that same OOM path.  I.e. this passes |delete| a bogus pointer,
  which could easily cause a crash if executed.
  
You'll need to look at the original code to understand the patches;  eight 
lines isn't enough context.
Attachment #659978 - Flags: review?(jorendorff)
Keywords: sec-audit
(Assignee)

Comment 2

5 years ago
Comment on attachment 659978 [details] [diff] [review]
Slightly improve some JSScript OOM handling.

jorendorff:  review ping.  The patch is small and this is an s-s bug.
Attachment #659978 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7689d7feb6
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1c7689d7feb6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: --- → mozilla18

Updated

5 years ago
status-firefox-esr10: --- → unaffected
Group: core-security
status-b2g18: --- → fixed
status-firefox-esr17: --- → wontfix
You need to log in before you can comment on or make changes to this bug.