Closed Bug 767349 Opened 12 years ago Closed 12 years ago

IonMonkey: All generated snapshots should be bailout friendly.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox14 - unaffected
firefox15 - unaffected
firefox16 - unaffected
firefox17 - ---

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Currently a few tests are failing when we try to ensure that all snapshots correspond to the reconstructed stack size.  We should fix these before landing as they are source of potential errors in the wild.

I am flagging this as a security sensitive bug because it may impact privacy or security by allocating a bad JS stack after a bailout.
This patch is highlighting the miss-formed snapshots & resume points which are not safe for bailouts.

I am not sure this is the best location to add such check since the size of the resume points are the source of the problem, and thus this is likely to indicate a badly ordered resume point creation or a missing current->pop().
Attachment #635724 - Flags: feedback?(dvander)
Comment on attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

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

Do we know why this is happening, or if it's possible to resume from these snapshots?
Attachment #635724 - Flags: feedback?(dvander) → feedback+
(In reply to David Anderson [:dvander] from comment #2)
> Do we know why this is happening, or if it's possible to resume from these
> snapshots?

I haven't investigated yet, my guess is that we are likely to resume from these snapshots because I don't recall any other reasons to encode a snapshot.
"allocating a bad JS stack after a bailout" sounds sec-criticalish, or am I misunderstanding?
(In reply to Daniel Veditz [:dveditz] from comment #4)
> "allocating a bad JS stack after a bailout" sounds sec-criticalish, or am I
> misunderstanding?

I don't know if it is really critical, but it might be if we can ever bailout with these resume points.
Keywords: sec-critical
Nicolas, can you take this bug? It sounds like you've done some preliminary investigation and if it's sg:crit we'll have to fix it.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Will track for 17 since that's the target for IonMonkey landing at present.
IonMonkey use js_ReconstructStack to ensure that bailouts are correct, the previous patch duplicate the assertion done during bailouts to ensure that all our snapshots & resume points are correct.

The following bug was marked as sec-critical because the previous patch was able to detect potential security involving the stack.  It appeared that the only failing bug in our test suite was related to and HIDDEN JSOP_ENDITER opcode.

This patch ensure that we are reconstructing the stack depth even for sequence of hidden instructions and recover the default stack depth if we are no longer on hidden instructions.
Attachment #645401 - Flags: review?(brendan)
Comment on attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

Once the patch fixing HIDDEN instruction in js_ReconstructStackDepth is applied, we can land this one as-is.
Attachment #635724 - Flags: review?(dvander)
Comment on attachment 645401 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

Load balance review request.
Attachment #645401 - Flags: review?(brendan) → review?(luke)
(patch is empty)
Here is the non-empty version of it.
Attachment #645401 - Attachment is obsolete: true
Attachment #645401 - Flags: review?(luke)
Attachment #645521 - Flags: review?(luke)
Comment on attachment 645521 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

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

::: js/src/jsopcode.cpp
@@ +6078,5 @@
> +        /* SRC_HIDDEN instructions are used for early-exit path and are not
> +         * keeping the stack depth.  As long as we are executing hidden
> +         * instructions we update the hidden pc depth instead of the pc depth.
> +         * Thus we can continue with the pc depth the next time we hit a
> +         * non-hidden instruction. */

This comment is a bit confusing.  How about:

/*
 * SRC_HIDDEN instructions annotate early-exit paths and do not affect the
 * stack depth when not taken. However, when pc points into an early-exit
 * path, hidden instructions need to be taken into account.
 */

@@ +6092,1 @@
>          if (SimulateOp(cx, script, op, cs, pc, pcstack, pcdepth) < 0)

Instead of using 'continue, can you put the non-hidden case in the else branch?  That would give the code a bit more symmetry.
Attachment #645521 - Flags: review?(luke) → review+
Comment on attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

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

Do we know why this is happening, or if it's possible to resume from these snapshots?
Attachment #635724 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #14)
> Do we know why this is happening, or if it's possible to resume from these
> snapshots?

What was happening is now fixed in the patch reviewed by luke.  JSOP_ENDITER opcode was flagged as hidden and js_ReconstructStackDepth did not iterate over hidden instructions.  So when you make a snapshot on an hidden instruction, you got an off-by-pushs/pops caused by the instruction which is reflected by the resume point but not by its counter part in the assertion.

So the current assertion highlighted an issue which was not in one of our resume point but in the js_ReconstructStackDepth function.  Knowing that this assertion is also in our bailout code, I will land it to catch more potential bailout mismatch.

js_ReconstructStackDepth was only used for debug and we don't have any use case elsewhere than in IonMonkey, so we can safely remove the sec-critical flag and open this bug to the public.
Group: core-security
Keywords: sec-critical
Comment on attachment 645521 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

https://hg.mozilla.org/projects/ionmonkey/rev/7f0f1fdfa5e2
Attachment #645521 - Flags: checkin+
Comment on attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

https://hg.mozilla.org/projects/ionmonkey/rev/2af804d84437
Attachment #635724 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 777653
You need to log in before you can comment on or make changes to this bug.