Closed Bug 552599 Opened 15 years ago Closed 6 years ago

[@ JS_Assert] JS_GetFrameScopeChain aborts when asked about a native frame


(Core :: JavaScript Engine, defect)

Not set





(Reporter: timeless, Assigned: dmandelin)



(Keywords: assertion, crash)

Crash Data


(1 obsolete file)

with luke's fix for bug 552248, and using the general steps from bug 552248 comment 11, i now hit this gem:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x0000000103b20065 in JS_Assert (s=0x103bbbee0 "FUN_INTERPRETED(this)", file=0x103ba44b8 "jsfun.h", ln=173)
73	    *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0 JS_Assert (s=0x103bbbee0 "FUN_INTERPRETED(this)", file=0x103ba44b8, ln=173)
#1 JSFunction::countArgsAndVars (this=0x1210c7af0)
#2 NewCallObject
#3 js_GetCallObject
#4 JS_GetFrameCallObject
#5 JS_GetFrameScopeChain
#6 jsd_GetScopeChainForStackFrame
#7 JSD_GetScopeChainForStackFrame

I believe the right regression for this is:

filed confidential per gal's request. there's a 1 week expiry window.
Attached patch proposal (obsolete) — Splinter Review
with this, i can play with the native frame in venkman without crashing, and the results seem fairly reasonable.

note that 'this' can easily be different between the caller frame and the native frame, and being able to see the other this is fairly helpful.
Assignee: general → timeless
Attachment #432771 - Flags: review?(dmandelin)
(note that i can't get bugmail for this bug while it's grouped, so if you have comments, please contact me by irc).
Timeless, you shouldn't have to patch this. Patching to whack bug-moles is no good either, for instance the jsfun.h changes are wrong and should not be committed -- but again this is no slam on you, since you were doing best effort under duress.

JS_EvaluateUCInStackFrame passes JS_DISPLAY_SIZE to JSCompiler::compileScript to try to shut off use of the display, but this doesn't satisfy the assertion and assumptions in the code hg blame'ed on dmandelin.

I think this bug should not be restricted. Let's fix it in the open. I'll leave that decision to sayrer.

Flags: in-testsuite?
dmandelin has the blame, to assigning there, but delegation is obv ok.
Assignee: timeless → dmandelin
Group: core-security
Comment on attachment 432771 [details] [diff] [review]

Looks good to me. I assume you know what you are doing with the JSFunction members being called on debugger-only stuff, since I don't know anything about that.
Attachment #432771 - Flags: review?(dmandelin) → review+
Comment on attachment 432771 [details] [diff] [review]

The jsfun.h changes are not wanted. We do not want to paper over bugs where a native function is used as if it were a scripted function.

Isn't the problem here that a native function call frame can be presented to JS_EvaluateUCInStackFrame?

Attachment #432771 - Flags: review-
I tried to repro this with the steps in bug 552248 comment 11, but at step 12, it doesn't crash, and it doesn't stop in venkman. Is '/fbreak cert 1' an abbreviation or something? Or are the steps a bit different for this bug?
brendan: it needs to be legal to do this, it's actually quite useful.
Timeless: can you say why it's useful? What about a native function call's frame is useful for eval-in-stack-frame? Is it a particular native (e.g., obj_eval) or any/many? More details would be helpful.

brendan: sorry, yes, being able to evaluate with the scope/object are useful, as well as poking the object's parent and getting other properties.

roughly, the useful things are:

scope + 'this object' + all properties relating to 'this object' (including parents)
Comment on attachment 432771 [details] [diff] [review]

while this patch has been shifting in my tree, i think it's now reached the completely rotted state, only two hunks have survived:

@@ -2267,9 +2267,12 @@ BindNameToSlot(JSContext *cx, JSCodeGene
-            JS_ASSERT(cg->staticLevel >= fun->u.i.script->staticLevel);
+            if (cg->staticLevel < fun->u.i.script->staticLevel)
+                return JS_TRUE;

@@ -187,7 +187,7 @@ Parser::Parser(JSContext *cx, JSPrincipa
-    JS_ASSERT_IF(cfp, cfp->isScriptFrame());
+    /* The debugger might ask us to operate on a native caller frame pointer */

I'm going to drop the patch from my queue. We'll see if I get around to using venkman this year...
Attachment #432771 - Attachment is obsolete: true
Crash Signature: [@ JS_Assert]
Closing because no crash reported since 12 weeks.
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.