If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Static analysis bug: js_Execute calling JS_REQUIRES_STACK js_FreeRawStack

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
JavaScript Engine
--
blocker
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
js_Execute is calling js_FreeRawStack which is JS_REQUIRES_STACK. This dates all the way back to http://hg.mozilla.org/mozilla-central/rev/932126be5356

I think this was only recently exposed due to a bug fix in lazy_types.js

I don't know the JS engine enough to know where the bug is here: should js_Execute be marked JS_REQUIRES_STACK? It gets called from JS_ExecuteScript which, I think, needs to bail off trace, but I'd like confirmation of that from somebody who knows better.
(Assignee)

Updated

9 years ago
Blocks: 476177
No longer blocks: 476166
Yes, js_Execute should be JS_REQUIRES_STACK. Then what's next? ;-)

/be
(Assignee)

Comment 2

9 years ago
Created attachment 376415 [details] [diff] [review]
js_Execute is JS_REQUIRES_STACK, and implications thereof, rev. 1

It only unravels a bit... but that seems to be an important bit!

This fixes the js_FreeRawStack error, but it does *not* fix another set of errors in js_CheckUndeclaredVarAssignment which appear to be more recent, and seem to be an analysis problem. I'll look at that separately.
Assignee: general → benjamin
Status: NEW → ASSIGNED
Attachment #376415 - Flags: review?(jorendorff)
I think it would be better (less code) to make js_Execute JS_FORCES_STACK and put the js_LeaveTrace call in there.

r=me with that.  Thanks for looking into these.
Attachment #376415 - Flags: review?(jorendorff) → review+
Comment on attachment 376415 [details] [diff] [review]
js_Execute is JS_REQUIRES_STACK, and implications thereof, rev. 1

(In reply to comment #3)
> I think it would be better (less code) to make js_Execute JS_FORCES_STACK and
> put the js_LeaveTrace call in there.

That works for this case:

> JS_ExecuteScript(JSContext *cx, JSObject *obj, JSScript *script, jsval *rval)
> {
>     JSBool ok;
> 
>     CHECK_REQUEST(cx);
>+    js_LeaveTrace(cx);
>     ok = js_Execute(cx, obj, script, NULL, 0, rval);
>     LAST_FRAME_CHECKS(cx, ok);
>     return ok;
> }

But what about this one, if there's a compilation error?

>@@ -5227,16 +5228,17 @@ JS_EvaluateUCScriptForPrincipals(JSConte
>                                  const jschar *chars, uintN length,
>                                  const char *filename, uintN lineno,
>                                  jsval *rval)
> {
>     JSScript *script;
>     JSBool ok;
> 
>     CHECK_REQUEST(cx);
>+    js_LeaveTrace(cx);
>     script = js_CompileScript(cx, obj, NULL, principals,
>                               !rval
>                               ? TCF_COMPILE_N_GO | TCF_NO_SCRIPT_RVAL
>                               : TCF_COMPILE_N_GO,
>                               chars, length, NULL, filename, lineno);
>     if (!script) {
>         LAST_FRAME_CHECKS(cx, script);
>         return JS_FALSE;

LAST_FRAME_CHECKS is tricky, it wants to do stuff if !cx->fp. Could we trigger a trace with cx->fp null, then from the trace call one of these APIs and end up misjudging cx->fp's null-ness? This isn't an issue now with traces triggered only from the interpreter, but it could be if we trigger from native code calling an event handler (event loop tracing).

/be
LAST_FRAME_CHECKS already calls JS_IsRunning, which does this:

    return JS_ON_TRACE(cx) || cx->fp != NULL;
Ok, common code is best all else equal (and correct!).

/be
(Assignee)

Comment 7

9 years ago
Created attachment 376431 [details] [diff] [review]
js_Execute should be JS_FORCES_STACK (and actually do it), rev. 2
Attachment #376415 - Attachment is obsolete: true
Attachment #376431 - Flags: review?(jorendorff)
Attachment #376431 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/98dab8153e42
http://hg.mozilla.org/tracemonkey/rev/250bd212fd59

This needs to land in 1.9.1 also: nesting script execution within native methods isn't uncommon.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: blocking1.9.1+
Resolution: --- → FIXED
Could this have fixed some of the random tinderbox orange?
(Assignee)

Comment 10

9 years ago
I'd be a bit surprised, but it's possible.
(Assignee)

Comment 11

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c27e4c8b850f
Keywords: fixed1.9.1
(Assignee)

Updated

9 years ago
Flags: in-testsuite+
Benjamin, which tests are you referring to? There are none included in the patches.
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 13

8 years ago
The static analysis caught the bug originally, so if it regressed the tree would turn red again. It's a build-time test.
Thanks Benjamin. The trees are green so we can safely mark this as verified fixed on trunk and 1.9.1.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
OS: Linux → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.