Closed
Bug 476177
Opened 15 years ago
Closed 15 years ago
TM: cx->stackPool must not be accessed on trace
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Keywords: verified1.9.1)
Attachments
(1 file, 2 obsolete files)
42.69 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
Bug 460865 put a write barrier around cx->fp. We need the exact same thing around cx->stackPool. Natives mostly touch cx->stackPool when allocating stack slots as a prelude to calling js_Invoke, so they're usually about to reenter the interpreter anyway. The danger is, if you allocate stack slots, *then* deep-bail, LeaveTree will free your stack slots as part of reconstructing the native stack. They could tehn get clobbered. (LeaveTree resets the stackPool to a mark. Incidentally this is really hard to diagnose when it happens. ;-) To avoid this danger you have to deep-bail first, then allocate the stack slots and go about your business. The good news is that the static analysis of bug 460865, which bsmedberg and dmandelin so kindly unbroke and maintained, is perfectly suited to the task of finding all the places where this is happening and fixing them. Patch coming.
Assignee | ||
Comment 1•15 years ago
|
||
The key change here is: > /* Stack arena pool and frame pointer register. */ > + JS_REQUIRES_STACK > JSArenaPool stackPool; Everything else is reconciling that with static analysis. Huge patch, but it's mostly just annotations and assertions. In a few places that's not enough, and we really need to call js_LeaveTrace, i.e. we must deep-bail if we get there on trace. This makes a few JSFastNatives JS_REQUIRES_STACK, a change I will have to revert in bug 463238. But I'll burn that bridge when I come to it...
Attachment #359775 -
Flags: review?(brendan)
Assignee | ||
Comment 2•15 years ago
|
||
Oops, forgot to qrefresh-- I'll also make this change in js_Invoke:
> - frame.down = js_GetTopStackFrame(cx);
> + frame.down = cx->fp;
Since js_Invoke is now JS_REQUIRES_STACK, it can access cx->fp directly. It does not need to call the read-barrier function anymore.
No longer blocks: deepbail
Assignee | ||
Comment 3•15 years ago
|
||
Interdiff below. v1 broke non-JS_TRACER platforms. (The hunk in comment 2 was in v1 after all, so it doesn't show up in the interdiff.) --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -80,12 +80,9 @@ #include "jsscript.h" #include "jsstr.h" +#include "jstracer.h" #include "jsdbgapi.h" #include "prmjtime.h" #include "jsstaticcheck.h" -#if !defined JS_THREADSAFE && defined JS_TRACER -#include "jstracer.h" -#endif - #if JS_HAS_FILE_OBJECT #include "jsfile.h" diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp --- a/js/src/jsobj.cpp +++ b/js/src/jsobj.cpp @@ -71,4 +71,5 @@ #include "jsscript.h" #include "jsstr.h" +#include "jstracer.h" #include "jsdbgapi.h" /* whether or not JS_HAS_OBJ_WATCHPOINT */ #include "jsstaticcheck.h" diff --git a/js/src/jstracer.h b/js/src/jstracer.h --- a/js/src/jstracer.h +++ b/js/src/jstracer.h @@ -580,11 +580,4 @@ extern JSObject * js_GetBuiltinFunction(JSContext *cx, uintN index); -static JS_INLINE JS_FORCES_STACK void -js_LeaveTrace(JSContext *cx) -{ - if (JS_ON_TRACE(cx)) - js_GetTopStackFrame(cx); -} - #else /* !JS_TRACER */ @@ -595,3 +588,10 @@ js_LeaveTrace(JSContext *cx) #endif /* !JS_TRACER */ +static JS_INLINE JS_FORCES_STACK void +js_LeaveTrace(JSContext *cx) +{ + if (JS_ON_TRACE(cx)) + js_GetTopStackFrame(cx); +} + #endif /* jstracer_h___ */
Attachment #359775 -
Attachment is obsolete: true
Attachment #359802 -
Flags: review?(brendan)
Attachment #359775 -
Flags: review?(brendan)
Assignee | ||
Comment 4•15 years ago
|
||
*sigh* I annotated some function definitions. With jsstack.js, when you do that, you have to match any matching declarations too. Otherwise you create a hole in the analysis. So v3 has an extra few annotations. The only non-annotation code changes are: - I made jstracer.cpp:js_DeleteRecorder static (we should rename it someday) - SPROP_GET and SPROP_SET now call js_LeaveTrace just before calling a scripted getter/setter. (we should convert those to inline functions someday)
Attachment #359802 -
Attachment is obsolete: true
Attachment #359839 -
Flags: review?(brendan)
Attachment #359802 -
Flags: review?(brendan)
Comment 5•15 years ago
|
||
Comment on attachment 359839 [details] [diff] [review] v3 Only comment is to push js_LeaveTrace down into js_Internal{Invoke,GetOrSet} since those are exactly the helpers that need to leave-trace. /be
Attachment #359839 -
Flags: review?(brendan) → review+
Comment 6•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/932126be5356
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: blocking1.9.1+
Resolution: --- → FIXED
Comment 8•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d3b3587362a4
Keywords: fixed1.9.1
Comment 9•15 years ago
|
||
verified 1.9.1, 1.9.2 based on green static analysis tinderbox for mozilla-central, tracemonkey.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•