Closed Bug 476177 Opened 15 years ago Closed 15 years ago

TM: cx->stackPool must not be accessed on trace


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: jorendorff, Assigned: jorendorff)



(Keywords: verified1.9.1)


(1 file, 2 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
The key change here is:

>      /* Stack arena pool and frame pointer register. */
>      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)
Blocks: deepbail
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
Attached patch v2 (obsolete) — Splinter Review
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"
 #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);
-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 */
+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)
Attached patch v3Splinter Review
*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 on attachment 359839 [details] [diff] [review]

Only comment is to push js_LeaveTrace down into js_Internal{Invoke,GetOrSet} since those are exactly the helpers that need to leave-trace.

Attachment #359839 - Flags: review?(brendan) → review+
Closed: 15 years ago
Flags: blocking1.9.1+
Resolution: --- → FIXED
covered by static analysis.
Flags: in-testsuite+
Flags: in-litmus-
verified 1.9.1, 1.9.2 based on green static analysis tinderbox for mozilla-central, tracemonkey.
Depends on: 492040
You need to log in before you can comment on or make changes to this bug.