Closed Bug 476177 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Keywords: verified1.9.1)

Attachments

(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. */
> +    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)
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"
-#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)
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]
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+
http://hg.mozilla.org/mozilla-central/rev/932126be5356
Status: NEW → RESOLVED
Closed: 14 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.
Status: RESOLVED → VERIFIED
Depends on: 492040
You need to log in before you can comment on or make changes to this bug.