IonMonkey: Enable StackIter::fp() assertion.

RESOLVED FIXED in Firefox 19

Status

()

--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

({assertion})

Trunk
mozilla19
x86_64
Linux
assertion
Points:
---

Firefox Tracking Flags

(firefox15 unaffected, firefox16 unaffected, firefox17 unaffected, firefox18 affected, firefox19 fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [ion:p1][adv-main19+])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Currently there is a remaining comment on top of StackIter::fp() asking for adding !isIon() to the assertion, one use case is comming from fun_getProperty and another is coming from pushExecuteFrame (under DebuggerFrame_eval).

I am haven't investigated all the risk yet, so I flagging this as s-s in the mean time and flag it for ff18.
Whiteboard: [ion:p1:fx18] → [ion:p1]
(Assignee)

Updated

6 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 668275 [details] [diff] [review]
Embed !isIon() assertion in StackIter::fp().

Add extra assertions which should not break, and remove invalid use cases of StackIter::fp().
Attachment #668275 - Flags: review?(luke)

Comment 2

6 years ago
Comment on attachment 668275 [details] [diff] [review]
Embed !isIon() assertion in StackIter::fp().

Great!
Attachment #668275 - Flags: review?(luke) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 668731 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

Ok, this is no longer the same size of patch, but this one brings more sanity and gave me the opportunity to review all uses cases.

jsdbgapi is remaining unchanged with one restriction which is that JSStackFrame returned by the JS_Broken* function are only non-Ion interpreter frames.
Attachment #668275 - Attachment is obsolete: true
Attachment #668731 - Flags: review?(luke)

Comment 6

6 years ago
Comment on attachment 668731 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

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

::: js/src/jsapi.cpp
@@ +7312,5 @@
>      ScriptFrameIter i(cx);
>      if (i.done())
>          return cx->global();
> +    RootedObject scopeChain(cx, i.scopeChain());
> +    return &scopeChain->global();

return &i.scopeChain()->global();

seems fine; global() is a simple loop.

::: js/src/jscntxtinlines.h
@@ -586,5 @@
>  }
>  
> -/* Get the current frame, first lazily instantiating stack frames if needed. */
> -static inline js::StackFrame *
> -js_GetTopStackFrame(JSContext *cx, FrameExpandKind expand)

Happy to see this old wart go.

::: js/src/jscompartment.cpp
@@ +672,5 @@
>  
>  bool
>  JSCompartment::hasScriptsOnStack()
>  {
> +    for (AllFramesIter f(rt->stackSpace); !f.done(); ++f) {

If you're going to make it longer, how about 'afi'?

@@ +678,5 @@
> +        // If this is an Ion frame, check the IonActivation instead
> +        if (f.fp()->beginsIonActivation())
> +            continue;
> +#endif
> +        if (f.fp()->script()->compartment() == this)

This code makes me think we need to rename AllFramesIter::fp to AllFramesIter::interpFrame and add AllFramesIter::isIon.

::: js/src/jsdbgapi.cpp
@@ +482,5 @@
> +        js::mjit::ExpandInlineFrames(cx->compartment);
> +#endif
> +        fp = cx->maybefp();
> +    } else
> +        fp = fp->prev();

{ } since the then-branch has 'em.

::: js/src/jsobj.cpp
@@ +4449,5 @@
>  JS_FRIEND_API(bool)
>  js::CheckUndeclaredVarAssignment(JSContext *cx, JSString *propname)
>  {
> +    JSScript *script = cx->stack.currentScript();
> +    if (!script)

I think you want currentScript(NULL, ALLOW_CROSS_COMPARTMENT).

::: js/src/shell/js.cpp
@@ +1383,5 @@
>      JS_ASSERT(!iter.done());
>  
> +    // interpFrame might assert that we are not using an IonMonkey
> +    // fake-interpreter frame. This would not be fix unless we want to support
> +    // IonMonkey frames in JS_EvaluateUCInStackFrame.

This comment is confusing.  How about "Debug-mode currently disables Ion compilation."

@@ +2508,5 @@
>      JS_ASSERT(cx->hasfp());
>  
> +    // interpFrame might assert that we are not using an IonMonkey
> +    // fake-interpreter frame. This would not be fix unless we want to support
> +    // IonMonkey frames in JS_EvaluateUCInStackFrame.

ditto

::: js/src/vm/Stack.cpp
@@ +1055,5 @@
>      if (evalInFrame) {
>          /* Though the prev-frame is given, need to search for prev-call. */
>          StackSegment &seg = cx->stack.space().containingSegment(evalInFrame);
>          StackIter iter(cx->runtime, seg);
> +        JS_ASSERT(!evalInFrame->beginsIonActivation());

Can you comment here "Debug-mode currently disables Ion compilation".  Then you should be able to take out the "|| iter.isIon".  That way we'll hit the assert eagerly if we ever add debug-mode instead and fix it correctly.

::: js/src/vm/Stack.h
@@ +1774,5 @@
> +    // the interpreter and by JagerMonkey. IonMonkey does not maintain this
> +    // interpreter frame but use one to indicate the presence of an activation
> +    // of IonMonkey code on the C stack. Such fake interpreter frame contains
> +    // mostly garbage, and so access to such frame is limited.
> +    StackFrame *interpFrame() const { JS_ASSERT(isScript() && !isIon()); return fp_; }

How about replacing the last two sentences with:
 "When entering IonMonkey, the top interpreter frame (pushed by the caller) is kept on the stack as bookkeeping (with beginsIonActivation() set).  The contents of the frame are ignored by Ion code (and GC) and thus immediately become garbage and must not be touched directly."
Attachment #668731 - Flags: review?(luke) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 669352 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

Compared to the previous version:
- Apply nits.
- Fix StackIter::* for the native case (use fp_ instead of interpFrame())
- Fix assertions (beginsIonActivation -> runningInIon), see Bug 798792 comment 4.  And fixing Bug 798792 issue too.
- Add StackIter::compartment for checking that all debugged frames are not Ion frames.
Attachment #668731 - Attachment is obsolete: true
Attachment #669352 - Flags: review?(luke)

Comment 8

6 years ago
Comment on attachment 669352 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

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

::: js/src/jscompartment.cpp
@@ +676,5 @@
> +    for (AllFramesIter afi(rt->stackSpace); !afi.done(); ++afi) {
> +#ifdef JS_ION
> +        // If this is an Ion frame, check the IonActivation instead
> +        if (afi.fp()->runningInIon())
> +            continue;

Again, could you rename AllFramesIter::fp to interpFrame and add a AllFramesIter::inIon() that can be used for this branch?

::: js/src/vm/Stack.cpp
@@ +1530,5 @@
> +        break;
> +#endif
> +      case SCRIPTED:
> +      case NATIVE:
> +        return fp_->compartment();

NATIVE shouldn't use fp_ (there might not be one or it might be in a different compartment).  Use  calls_->callee().compartment() for the NATIVE case instead.
Attachment #669352 - Flags: review?(luke) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 669861 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

Delta:
- Fix StackIter Native's compartment.
- Rename AllFrameIter::fp() to AllFrameITer::interpFrame().
- Filter out Ion frames from AllFrameIter iterations, because they are either used for debug or handled in other path (such as jscompartment.cpp & jsscript.cpp)

\o/ No more un-garded usage of “StackFrame *” which is an Ion activation frame.
Thanks for the renaming suggestion.
Attachment #669352 - Attachment is obsolete: true
Attachment #669861 - Flags: review?(luke)

Comment 10

6 years ago
Comment on attachment 669861 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

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

Nice job wading through all this and fixing the next 5 fuzz bugs :)

::: js/src/ion/IonFrameIterator.h
@@ +301,5 @@
>          return si_;
>      }
>      bool isFunctionFrame() const;
>      bool isConstructing() const;
> +    JSObject *scopeChainObject() const;

scopeChain() would be shorter and more consistent.

::: js/src/jsscript.cpp
@@ +2572,5 @@
>       *  - type inference data for the script assuming script->needsArgsObj; and
>       */
>      for (AllFramesIter i(cx->stack.space()); !i.done(); ++i) {
> +        if (i.isIon())
> +            continue;

How about a comment on this to the effect of:

We cannot reliably create an arguments object for Ion activations of this script.  To maintain the invariant that "script->needsArgsObj implies fp->hasArgsObj", the Ion bail mechanism will create an arguments object right after restoring the StackFrame and before entering the interpreter.  This delay is safe since the engine avoids any observation of a StackFrame when it beginsIonActivation (see StackIter::interpFrame comment).

::: js/src/vm/Debugger.cpp
@@ +1761,5 @@
>       * cx->fp() would return the topmost frame in the current context.
>       * Since there may be multiple contexts, use AllFramesIter instead.
>       */
>      for (AllFramesIter i(cx->stack.space()); !i.done(); ++i) {
> +        /* Debug-mode currently disables Ion compilation. */

By itself, this comment would seem to indicate that we shouldn't find any i.isIon() frames.  Perhaps a slight expansion: "Debug-mode currently disables Ion compilation in the compartment of the debuggee." ?

::: js/src/vm/ScopeObject.cpp
@@ +1835,5 @@
>       */
>      for (AllFramesIter i(cx->runtime->stackSpace); !i.done(); ++i) {
> +        /* Debug-mode currently disables Ion compilation. */
> +        if (i.isIon())
> +            continue;

ditto

::: js/src/vm/Stack.cpp
@@ +825,5 @@
>  {
>      for (AllFramesIter i(*this); !i.done(); ++i) {
> +        /* Debug-mode currently disables Ion compilation. */
> +        if (i.isIon())
> +            continue;

ditto

::: js/src/vm/Stack.h
@@ -1527,5 @@
>      /* The StackSpace currently hosting this ContextStack. */
>      StackSpace &space() const { return *space_; }
>  
> -    /* Return whether the given frame is in this context's stack. */
> -    bool containsSlow(const StackFrame *target) const;

nice catch
Attachment #669861 - Flags: review?(luke) → review+
(Assignee)

Comment 11

6 years ago
My dev-branch on try, before rebasing and fixing a minor merge issue between 2 of my patches (in js/src/vm/ArgumentsObject.cpp):
https://tbpl.mozilla.org/?tree=Try&rev=98d875f58193

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebeca12019a2
https://hg.mozilla.org/mozilla-central/rev/ebeca12019a2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Updated

6 years ago
Duplicate of this bug: 798792
What's the rating on this bug? Do we think that B2G is impacted?
status-firefox-esr17: --- → unaffected
(Assignee)

Comment 15

6 years ago
(In reply to Alex Keybl [:akeybl] from comment #14)
> What's the rating on this bug? Do we think that B2G is impacted?

This patch should hopefully not change any behavior.  So I don't think is is needed for B2G.
status-b2g18: --- → unaffected
Whiteboard: [ion:p1] → [ion:p1][adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.