451322 - TM: LirBufWriter does not NULL check

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 1

[Damon Sicore (:damons)]

blocking1.9.1+, needs a beta vector, P2, per triage w/ Brendan, Andreas, and danderson.

Comment 2

[Jesse Ruderman]

dvander thinks this might lead to memory corruption, so marking security-sensitive.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: nanojit fixes

Fixes some error handling in nanojit

Comment 6

[Rick Reitmaier]

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() )

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: nanojit fixes v2

No reason in particular, here's the same patch without the bool return.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Pushed changeset http://hg.mozilla.org/tracemonkey/rev/a9db6bedd33d - still need to work out allocation failures during recording though.

Comment 9

[Rick Reitmaier]

see https://bugzilla.mozilla.org/show_bug.cgi?id=461073 for nj mem issues.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-451322.js

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed mozilla-central, tracemonkey.

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.1

Comment 14

[Bob Clary [:bc] (inactive)]

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

[Andreas Gal :gal]

Investigating.

Comment 16

[Bob Clary [:bc] (inactive)]

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.

Comment 17

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-451322.js marked skip.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fac742496b10
http://hg.mozilla.org/tracemonkey/rev/6c3f2e826dec