Closed Bug 635811 Opened 11 years ago Closed 11 years ago

prevent call objects from escaping on error paths


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: luke, Assigned: luke)



(Whiteboard: [hardblocker] [has patch])


(2 files, 1 obsolete file)

Once Invoke/Execute/etc have created the call object for a frame, PutActivationObjects must be called.  Currently, this is done in ScriptEpilogue, but if there is an error before ScriptPrologue is called, then ScriptEpilogue is not called.  This was the source of bug 634452.  I think the fix is just to move the PutActivationObjects call out of ScriptEpilogue and into the frame-popping logic.  The tricky part is making sure the mjit/tjit transition code does the right thing.  Doing this will also avoid the temporary hack needed to fix bug 635805.
Attached patch fix the hack (obsolete) — Splinter Review
Ah, gag, thanks Jesse.  Bug 635805 is going to fix all this and make this explicit PutActivationObjects business unnecessary, but might as well fix the temporary fix first.
Attachment #514119 - Flags: review?(gal)
Attachment #514119 - Flags: review?(gal)
Oops, disregard comment 1; wrong bug.
Another rare corner case that should be fixed by this patch is that of activation objects not being put in some cases of generator closing.
Attachment #514119 - Flags: review+
Attachment #514119 - Attachment is obsolete: true
Attached patch assert of doomSplinter Review
If I apply this patch (which simply asserts !fp->hasCallObj() on exiting RunScript) I get oodles of failures in the current jit tests.  I am going to forgo the beautifying fat stack of patches I was cooking up yesterday in lieu of a simple spot fix which can land for 4.0 (bug 635599 is a final+ hardblocker and a dup of this).
Duplicate of this bug: 635599
blocking2.0: --- → final+
Whiteboard: [hardblocker]
Attached patch fixSplinter Review
Attachment #514623 - Flags: review?(dvander)
Whiteboard: [hardblocker] → [hardblocker] [has patch]
Attachment #514623 - Flags: review?(dvander) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.