Last Comment Bug 767349 - IonMonkey: All generated snapshots should be bailout friendly.
: IonMonkey: All generated snapshots should be bailout friendly.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 777653
Blocks: IonMonkey
  Show dependency treegraph
 
Reported: 2012-06-22 06:26 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-09-04 10:40 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
unaffected
-
unaffected
-
unaffected
-


Attachments
Highlight wrong snapshots/resume points. (1.43 KB, patch)
2012-06-22 06:33 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
dvander: feedback+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review
Simluate hidden instruction when the target is hidden. (342 bytes, patch)
2012-07-24 11:34 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Simluate hidden instruction when the target is hidden. (2.51 KB, patch)
2012-07-24 14:53 PDT, Nicolas B. Pierron [:nbp]
luke: review+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-06-22 06:26:36 PDT
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.
Comment 1 Nicolas B. Pierron [:nbp] 2012-06-22 06:33:07 PDT
Created attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

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().
Comment 2 David Anderson [:dvander] 2012-06-22 15:40:14 PDT
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?
Comment 3 Nicolas B. Pierron [:nbp] 2012-07-02 01:45:29 PDT
(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.
Comment 4 Daniel Veditz [:dveditz] 2012-07-18 10:49:09 PDT
"allocating a bad JS stack after a bailout" sounds sec-criticalish, or am I misunderstanding?
Comment 5 Nicolas B. Pierron [:nbp] 2012-07-18 11:37:41 PDT
(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.
Comment 6 David Anderson [:dvander] 2012-07-18 11:52:02 PDT
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.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-23 15:06:41 PDT
Will track for 17 since that's the target for IonMonkey landing at present.
Comment 8 Nicolas B. Pierron [:nbp] 2012-07-24 11:34:08 PDT
Created attachment 645401 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

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.
Comment 9 Nicolas B. Pierron [:nbp] 2012-07-24 11:36:35 PDT
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.
Comment 10 Nicolas B. Pierron [:nbp] 2012-07-24 11:38:13 PDT
Comment on attachment 645401 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

Load balance review request.
Comment 11 Luke Wagner [:luke] 2012-07-24 14:18:27 PDT
(patch is empty)
Comment 12 Nicolas B. Pierron [:nbp] 2012-07-24 14:53:53 PDT
Created attachment 645521 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

Here is the non-empty version of it.
Comment 13 Luke Wagner [:luke] 2012-07-24 15:01:51 PDT
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.
Comment 14 David Anderson [:dvander] 2012-07-24 16:35:30 PDT
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?
Comment 15 Nicolas B. Pierron [:nbp] 2012-07-24 17:26:58 PDT
(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.
Comment 16 Nicolas B. Pierron [:nbp] 2012-07-24 17:28:06 PDT
Comment on attachment 645521 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

https://hg.mozilla.org/projects/ionmonkey/rev/7f0f1fdfa5e2
Comment 17 Nicolas B. Pierron [:nbp] 2012-07-24 17:28:30 PDT
Comment on attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

https://hg.mozilla.org/projects/ionmonkey/rev/2af804d84437

Note You need to log in before you can comment on or make changes to this bug.