TM: LirBufWriter does not NULL check

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

({verified1.9.1})

unspecified
x86
Mac OS X
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x -
wanted1.8.1.x -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post 1.9.0)

Attachments

(4 attachments, 1 obsolete attachment)

ensureRoom() can return false and LirBufWriter does not check this.  When out of memory occurs things start crashing.  This could be problematic on mobile.

Updated

10 years ago
Assignee: general → danderson
Priority: -- → P3
blocking1.9.1+, needs a beta vector, P2, per triage w/ Brendan, Andreas, and danderson.
Flags: blocking1.9.1+
Priority: P3 → P2

Comment 2

10 years ago
dvander thinks this might lead to memory corruption, so marking security-sensitive.
Group: core-security
Created attachment 345649 [details] [diff] [review]
some fixes

LIR.cpp now checks ensureRoom() failures and returns the beginning of the buffer for safety.  We no longer start recorders if the LIR buffer can't get at least one page.  In MonitorRecording we check if the LIR buffer is out of memory, and we check the assembler state in a few more places.

The assembler checks its own state a little more aggressively since it asserted in a few places.
Attachment #345649 - Flags: review?(gal)
Created attachment 345650 [details] [diff] [review]
test case

test case that crashes without the above patch.  this particular crash is not exploitable.  I can't determine if nanojit's (somewhat messy) buffer handling could be exploited in out of memory conditions.  Given how easy it is to hit that (24MB code cache), I asked Jesse for security sensitivity just in case.

Updated

10 years ago
Attachment #345649 - Flags: review?(gal) → review+
Created attachment 346119 [details] [diff] [review]
nanojit fixes

Fixes some error handling in nanojit
Attachment #346119 - Flags: review?(rreitmai)

Comment 6

10 years ago
re: nanojit fixes

Is there any reason why you don't want to call Assembler.error() and instead have beginAssembly() return a value? 

I'd rather keep the begin/assemble/end function free of return-values and provide a single consistent means to check for errors (i.e. error() )
Created attachment 346153 [details] [diff] [review]
nanojit fixes v2

No reason in particular, here's the same patch without the bool return.
Attachment #346119 - Attachment is obsolete: true
Attachment #346153 - Flags: review?(rreitmai)
Attachment #346119 - Flags: review?(rreitmai)

Updated

10 years ago
Attachment #346153 - Flags: review?(rreitmai) → review+
Pushed changeset http://hg.mozilla.org/tracemonkey/rev/a9db6bedd33d - still need to work out allocation failures during recording though.
Marking this particular bug as fixed.  We still have issues with LIR running out during recording, for example, lowering the code cache and running crypto-md5 will assert in a filter somewhere.  I'll defer to bug 461073
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 11

10 years ago
Created attachment 349423 [details]
js1_5/Regress/regress-451322.js

Updated

10 years ago
Flags: in-testsuite+
Flags: in-litmus-

Comment 12

10 years ago
verified fixed mozilla-central, tracemonkey.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1

Comment 13

9 years ago
v 1.9.1
Keywords: fixed1.9.1 → verified1.9.1

Comment 14

9 years ago
note js1_5/Regress/regress-451322.js is now asserting Assertion failed: "Unknown branch type in nPatchBranch": 0 due to bug 468782/http://hg.mozilla.org/tracemonkey/rev/ece63b96379b on tracemonkey and mozilla-central.

jason, see the test in comment 11.

Comment 15

9 years ago
Investigating.
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] post 1.9.0
Group: core-security

Comment 16

8 years ago
test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge. This timed out on tracemonkey debug builds. I'll mark it skip later today.
You need to log in before you can comment on or make changes to this bug.