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)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file)
1.59 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8506038 -
Attachment is patch: true
Comment 2•10 years ago
|
||
(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 3•10 years ago
|
||
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+
Updated•10 years ago
|
Group: core-security
Assignee | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd335d13677
Comment 5•10 years ago
|
||
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.
Description
•