Closed Bug 39141 Opened 24 years ago Closed 24 years ago

Omitting values or putting comments in array Literals cause crash; i.e. [1,,2] or [1,/*comment*/,2]

Categories

(Core :: JavaScript Engine, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: eXv, Assigned: rogerl)

References

()

Details

(Keywords: crash, Whiteboard: [nsbeta3+])

Attachments

(1 file)

Don't know quite how I stumbled upon this, but:

If you have this:

[1, /* comment */, 2] 

The interpreter crashes. Note the extra
comma there. Without the comma, it does not
crash. This does not affect Communicator (4.61)

I'm using build 2000051120.
I have also tested it under: 

JavaScript-C 1.5 pre-release 1 1999 10 31

And that crashes as well.


The following do not crash:

[1, /*ccc*/ 2]
[2 /* ewrew */, 1]

print( ({prop: /* value */ "real value"}).prop);

["one",,,,/**/,2] <-- strangely, does not crash



The following do crash:

[ ,1,2,3]  <-- i guess this should be just a syntax error maybe? But, in 4.61
it's just a null first elem.

["one",/**/,,2]

[/* comment */, 1,2,3]


The URL provided has the same info, but it has a link that eval()'s the
code so you can see whether it crases you.
Adding crash keyword
Keywords: crash
Confirming. Reproduced on PC/Linux with build 2000050811.
Stack trace from a 4/28 build (clicking on the link in the attachment):
#0  js_Interpret ()
#1  js_Execute ()
#2  obj_eval ()
#3  js_Invoke ()
#4  js_Interpret ()
#5  js_Invoke ()
#6  js_Interpret ()
#7  js_Execute ()
#8  JS_EvaluateUCScriptForPrincipals ()
#9  nsJSContext::EvaluateString ()
#10 nsEvaluateStringProxy::EvaluateString ()
#11 XPTC_InvokeByIndex ()
#12 EventHandler ()
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity is critical.
Severity: normal → critical
Status: NEW → ASSIGNED
Taking bugs off Roger's plate
Assignee: rogerl → mccabe
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Please see bug #47390. 
I guess problem is not about embedded comments : 
null (or omitted) values in array literals always cause crash. 
Simply try and enjoy : 
var myArray= ["Laurel",,"and","Hardy"]; 
A major bug, as this has always been considered as correct syntax.  
*** Bug 47390 has been marked as a duplicate of this bug. ***
Updating summary 
Summary: Some Comments in array Literals cause crash [1, /* comment */ ,2] → Omitting values or putting comments in array Literals cause crash; i.e. [1,,2] or [1,/*comment*/,2]
All these crashing examples involving comments 
involve omitted values too.  
Embedded comments without omitted values don't crash.  
Omitted values without embedded comments always crash.  
Regression from supported JS behavior, nominating nsbeta3.
Keywords: nsbeta3
Crashing regression --> nsbeta3+.
Whiteboard: [nsbeta3+]
The code for building the element initializer list gets confused whenever the 
initial comma token has been unget'd. The current token is comma, but passing 
&CURRENT_TOKEN(ts) to NewParseNode gets the preceding token since CURRENT_TOKEN 
doesn't allow for any lookahead tokens. This can be fixed by changing 
CURRENT_TOKEN to handle lookahead, but that seems like quite a significant 
change so I'm leaving it to the parser gods to decide...

Here's the patch:

diff -u -r3.13 jsscan.h
--- jsscan.h    2000/07/10 20:23:54     3.13
+++ jsscan.h    2000/08/15 22:34:44
@@ -168,7 +168,7 @@
     void                *listenerTSData;/* listener data for this TokenStream 
*/
 };

-#define CURRENT_TOKEN(ts) ((ts)->tokens[(ts)->cursor])
+#define CURRENT_TOKEN(ts) ((ts)->tokens[(ts->cursor + ts->lookahead) & 
NTOKENS_MASK])

 /* JSTokenStream flags */
 #define TSF_ERROR       0x01            /* fatal error while compiling */
Fix looks good to me.  How did CURRENT_TOKEN ever work, given that it was
missing '& NTOKENS_MASK' ?

Some brendan-nits -
You could parenthesize (ts) in the macro, to guard against unintended
interactions with the macro arguments.  Also, most if JS is 80-column-clean. 
You could use 

#define CURRENT_TOKEN(ts) ((ts)->tokens[((ts)->cursor + (ts)->lookahead)\
                                       & NTOKENS_MASK])

instead.

r=mccabe, assigning to rogerl to check in fix.
Assignee: mccabe → rogerl
Status: ASSIGNED → NEW
With nits out, checked in.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
I backed out the fix because it was breaking the Linux & Sparc builds. Turns out 
that adding lookahead in CURRENT_TOKEN is not the right change (I think mccabe 
was on the right track when he asked 'how did this ever work'?). After 
staring at the source a whole bunch more I convinced myself that the interaction 
between CURRENT_TOKEN & lookahead is correct. 

The original problem can be fixed more locally by:
diff -u -r3.38 jsparse.c
--- jsparse.c   2000/05/15 03:54:50     3.38
+++ jsparse.c   2000/08/18 21:27:07
@@ -2439,8 +2439,11 @@
                     break;
                 }

-                if (tt == TOK_COMMA)
+                if (tt == TOK_COMMA) {
                     pn2 = NewParseNode(cx, &CURRENT_TOKEN(ts), PN_NULLARY);
+                    /* Current token is actually the token behind the comma */
+                    pn2->pn_type = TOK_COMMA;
+                }
                 else
                     pn2 = AssignExpr(cx, ts, tc);
                 if (!pn2)

Excepting that this seems kind of 'hacky' it does fix the bug. Mike, can you r= 
this (and even better suggest a less hacky alternative?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think the above will work.  r=?

I'd be happy to check it in.
Fix checked in.
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: