Closed Bug 1033020 Opened 5 years ago Closed 5 years ago

Make BaslineFrame::returnValue match InterpreterFrame::returnValue

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- unaffected
firefox32 --- wontfix
firefox33 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main33+])

Attachments

(1 file, 1 obsolete file)

Right now, if you access BaselineFrame::returnValue and !hasReturnValue, you get random stack data (note, this should at least have had an assertion). On the other hand, it is safe to call InterpreterFrame::returnValue whenever: it will just give back UndefinedValue. This makes AbstractFramePointer's wrapper have inconsistent behavior. In particular, Debugger::slowPathOnLeaveFrame was not checking hasReturnValue, causing exception aborts in the debugger with jit frames on stack to read random memory from the stack as a Value.

I think the right fix here is to make both frame classes behave the same.
Attachment #8448970 - Flags: review?(jdemooij)
Blocks: GC.stability
Missed one subtlety: in setReturnValue, we need to set the value before the flag so that HandleValue doesn't see garbage in the constructor.
Attachment #8448970 - Attachment is obsolete: true
Attachment #8448970 - Flags: review?(jdemooij)
Attachment #8449027 - Flags: review?(jdemooij)
Comment on attachment 8449027 [details] [diff] [review]
missing_hasReturnValue_check-v1.diff

Review of attachment 8449027 [details] [diff] [review]:
-----------------------------------------------------------------

Good find!
Attachment #8449027 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/c5e0699e4b50
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Whiteboard: [adv-main33+]
Looks like this came from a code audit. If a test case appears, let me know so I can verify this bug. Otherwise, marking [qe-verify-].
Flags: qe-verify-
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.