Closed Bug 1083716 Opened 10 years ago Closed 10 years ago

This value for scripts not marked in Ion frame

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

MarkIonJSFrame() does not call MarkActualArguments() for scripts because scripts don't have arguments.  However this function also marks the 'this' value too which is not done in this case.

nbp said this value should not be used after execution of the script has started, but this appears to be happening in the following case:

- Ion code runs
- GC is triggered (via string concatenation called from Ion code)
- GC invalidates Ion code on the stack
- GC finishes and returns
- invaidated Ion code bails out to Baseline
- Baseline touches this value

This shows up as a crash with compacting GC because we now can move the object referenced by the this value.

I'm erring on the side of caution in marking this security relevant, but I don't think this is actually exploitable in released versions.
This patch fixes the problem and passes all tests, but I guess that maybe it is not the correct fix if this situation shouldn't happen in the first place.
Attachment #8506038 - Flags: feedback?(nicolas.b.pierron)
Attachment #8506038 - Attachment is patch: true
(In reply to Jon Coppeard (:jonco) from comment #0)
> I'm erring on the side of caution in marking this security relevant, but I
> don't think this is actually exploitable in released versions.

Nice catch. I think the |this| value here can only be the global, which should be tenured and marked anyway, but it can't hurt to be cautious.
Comment on attachment 8506038 [details] [diff] [review]
always-trace-frame-args

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

As Jandem mentioned, as the Global is tenured, it never moved and we never noticed any issues.
So I think it is safe to open this bug.

::: js/src/jit/IonFrames.cpp
@@ +868,5 @@
>  }
>  #endif
>  
>  static void
>  MarkActualArguments(JSTracer *trc, const JitFrameIterator &frame)

rename this function to MarkFrameAndActualArguments.

@@ +875,2 @@
>  
>      size_t nargs = frame.numActualArgs();

nit: MOZ_ASSERT(!CalleeTokenIsFunction(layout->calleeToken()), nargs == 0);

comment:
  // The trampoline produced by |generateEnterJit| is pushing |this| on the stack,
  // as requested by |setEnterJitData|.  Thus, this function is also used for marking
  // the |this| value of the top-level frame.
Attachment #8506038 - Flags: feedback?(nicolas.b.pierron) → review+
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/5dd335d13677
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.