The default bug view has changed. See this FAQ.

IonMonkey: All generated snapshots should be bailout friendly.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

(Blocks: 1 bug)

unspecified
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14- unaffected, firefox15- unaffected, firefox16- unaffected, firefox17-)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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().
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+
(Assignee)

Comment 3

5 years ago
(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?
(Assignee)

Comment 5

5 years ago
(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)

Updated

5 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED

Updated

5 years ago
status-firefox14: --- → unaffected
status-firefox15: --- → unaffected
status-firefox16: --- → unaffected
tracking-firefox14: --- → -
tracking-firefox15: --- → -
tracking-firefox16: --- → -
tracking-firefox17: --- → ?
Will track for 17 since that's the target for IonMonkey landing at present.
tracking-firefox17: ? → +
(Assignee)

Comment 8

5 years ago
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.
Attachment #645401 - Flags: review?(brendan)
(Assignee)

Comment 9

5 years ago
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)
(Assignee)

Comment 10

5 years ago
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)

Comment 11

5 years ago
(patch is empty)
(Assignee)

Comment 12

5 years ago
Created attachment 645521 [details] [diff] [review]
Simluate hidden instruction when the target is hidden.

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 13

5 years ago
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+
(Assignee)

Comment 15

5 years ago
(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
(Assignee)

Comment 16

5 years ago
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+
(Assignee)

Comment 17

5 years ago
Comment on attachment 635724 [details] [diff] [review]
Highlight wrong snapshots/resume points.

https://hg.mozilla.org/projects/ionmonkey/rev/2af804d84437
Attachment #635724 - Flags: checkin+
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 777653

Updated

5 years ago
tracking-firefox17: + → -
You need to log in before you can comment on or make changes to this bug.