Closed Bug 131510 Opened 23 years ago Closed 23 years ago

Crash when |arguments| defined as a variable inside function

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: ibukanov8, Assigned: brendan)

Details

(Keywords: crash, js1.5)

Attachments

(2 files, 3 obsolete files)

best fix, but more lines changed: exclude initializer for JSOP_ARGUMENTS
2.81 KB, patch
Details | Diff | Splinter Review
cvs diff -wu15 version of last patch
2.82 KB, patch
khanson
: review+
jband_mozilla
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
The following script crashes CVS version from 2002-03-17 of JS engine:

function f() {
		print("arguments="+arguments);
		var arguments;
}

f();

The js was compiled on RedHat 7.2 i386 via make -f Makefile.ref
The script crashes Mozilla 0.9.9 a well, like in

<html>
<body>
<script>
function f() {
	alert("arguments="+arguments);
	var arguments;
}

f();
</script>
</body>
</html>
Confirming crash in JS shell on WinNT; OS: Linux ---> All.
The crash can be reduced to this:

             function f() {var arguments;}()


WINNT STACK TRACE:

js_CheckRedeclaration(JSContext * 0x00301d60, JSObject * 0x00000000, long                
3151712, unsigned int 5, int * 0x0012eae4) line 1107 + 23 bytes
js_Interpret(JSContext * 0x00301d60, long * 0x0012fed8) line 3246 + 37 bytes
js_Execute(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript * 0x00306e10, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 968+ 13 bytes
JS_ExecuteScript(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript *                           
0x00306e10, long * 0x0012fed8) line 3258 + 25 bytes
Process(JSContext * 0x00301d60, JSObject * 0x002fb340, char * 0x00000000)
                                                          line 371 + 22 bytes
ProcessArgs(JSContext * 0x00301d60, JSObject * 0x002fb340, char * * 0x00300054,                                                    
int 0) line 529 + 17 bytes
main(int 0, char * * 0x00300054) line 2129 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1b9ea()


FUNCTION AT CRASHPOINT:

js_CheckRedeclaration(JSContext *cx, JSObject *obj, jsid id, uintN attrs,
                      JSBool *foundp)
{
    JSObject *obj2;
    JSProperty *prop;
    JSBool ok;
    uintN oldAttrs, report;
    JSBool isFunction;
    jsval value;

    if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop)) <<<<<<<< CRASHED HERE
        return JS_FALSE;

           etc.
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, js1.5
OS: Linux → All
Summary: carsh when arguments is defined after their use → Crash when |arguments| defined as a variable inside function
Typo: let's try that again. Crash can be reduced to

               function f() {var arguments;}; 
               f();
Testcase added to JS testsuite:

          mozilla/js/tests/js1_5/Regress/regress-131510-001.js

Currently passing in rhino, rhinoi shells.
Currently crashing in smdebug, smopt shells.
I think this bug has been around since the advent of lightweight functions. 
Can I have testing confirmation, r= and sr= for 1.0?

/be
I'll take this since I'm patching.

/be
Assignee: khanson → brendan
Keywords: mozilla1.0
Target Milestone: --- → mozilla1.0
Status: NEW → ASSIGNED
Priority: -- → P1
Why make a function heavyweight if it doesn't set arguments, but merely
mentions it in a var statement?  We already specialize JSOP_NAME to
JSOP_ARGUMENTS for such non-defining uses, but as the crash stack shows, we
mistakenly emit a JSOP_DEFVAR opcode, which requires a heavyweight function (so
that when called, fp->varobj will be a non-null ref to a Call object).	This
patch restores symmetry with the JSOP_NAME special case for JSOP_ARGUMENTS.

/be
Attachment #74593 - Attachment is obsolete: true
The latest patch passes the JS testsuite in the debug/optimized
JS shell on WinNT: I got only the known test failures. Furthermore,
the testcase for this bug passed -

I'll wait for this one to go in before I do the JS1.5 RC4a tarball.
Still hoping for r=khanson, sr=jband today.

/be
Attachment #74595 - Attachment is obsolete: true
brendan: I'm happy to sr= the main fix here (asuming a review by someone who's
dug into the jsemit code a little deeper than I). But that __GNUC__ bit seems
funky to me. Setting aside implicit knowledge of the invariants of the system,
you are actually doing something that *appears* to change the logic here. I
don't see why that would be required. What is the warning? Can't it be fixed in
a way that at least does not *appear* to be as invasive?
Perhaps a wider context diff will make it clear: gcc is complaining that
atomIndex may be used before set, but it is not used of op == JSOP_ARGUMENTS,
and op does not change.  So the __GNUC__-only setting of atomIndex = 0 has no
effect other than to stifle gcc.  Or so I say -- what makes you write "...
*appears* to change the logic here" -- in particular, what logic depends on
atomIndex == 0 or atomIndex != 0?

/be
jband's comment caused me to make the larger change I'd shied away from, but
the logic is all easy to see.  If LookupArgOrVar returns with pn->pn_op
optimized to JSOP_ARGUMENTS, then this must be a use, not a set, of 'arguments'
in a context where that name is not ambiguous.	No need to worry about
pn2->pn_expr, which if non-null is the variable's initializer, but which will
be null for this (op == JSOP_ARGUMENTS) case.

diff -wu15 coming up for review ease.

/be
Attachment #74831 - Attachment is obsolete: true
brendan: I'll look at the new patch. The point I was trying to make was that the
previous patch was in effect:

-            if (pn2->pn_slot >= 0) {
+            if (op != JSOP_ARGUMENTS && pn2->pn_slot >= 0) {

That may not be bad, but *appeared* deeper than the comment suggested.

Comment on attachment 74899 [details] [diff] [review]
cvs diff -wu15 version of last patch

r=khanson

The final logic looks clean to me.
Attachment #74899 - Flags: review+
Comment on attachment 74899 [details] [diff] [review]
cvs diff -wu15 version of last patch

sr=jband
Attachment #74899 - Flags: superreview+
Comment on attachment 74899 [details] [diff] [review]
cvs diff -wu15 version of last patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74899 - Flags: approval+
Fixed.

/be
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Marking Verified - the above testcase passes in the debug/optimized
JS shell on WinNT, Linux, and Mac9.1
Status: RESOLVED → VERIFIED
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: