53614 - Crash on calling a regular expression

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Waldemar Horwat]

The following crashes jsref:

/a.*b/("axb")

Comment 1

[rogerl (gone)]

This crashes because JSOP_PUSHOBJECT assumes that 'obj' has been loaded with the 
object to use as the this pointer while setting up the argument stack. In fact, 
'obj' is random stuff since the preceding opcode was JSOP_OBJECT which only set 
rval. Adding 
            obj = JSVAL_TO_OBJECT(rval);
after line 2741, jsinterp.c
            PUSH_OPND(rval);
fixes it, but I need a interpreter savvy fellow to say if this is the right fix.

Comment 2

[Blake Ross]

*spam*

adding crash keyword...

Comment 3

[Phil Schwartau]

Crash also occurs on WinNT; changing OS to "All". As per discussion 
with JS Engine team, setting P2 priority and rtm keyword

Comment 4

[rogerl (gone)]

Attachment: As a patch for review...

Comment 5

[rogerl (gone)]

looking for comments, or r,a= ??

Comment 6

[Patrick C. Beard]

Need to get this in, to prevent regular expression clients from crashing the 
browser. A patch is provided.

Comment 7

[rogerl (gone)]

r=,a= ??

Comment 8

[Michael La Guardia]

marking need info.  Please get a review and super review and then renominate

Comment 9

[Brendan Eich [:brendan]]

I thought JSOP_OBJECT set obj, but I must be senile.  a=[e-mail], 
[e-mail] should r= fast.

/be

Comment 10

[Brendan Eich [:brendan]]

Well, in the old days, JSOP_OBJECT did clear obj.  But for some reason, lost in 
the mist of the /m/src/ns inside netscape.com, rev 3.2 removed that obj clearing 
(fur did the deed, but I don't know who removed the obj = NULL; on the 
JSFUN13_BRANCH in the ns tree -- I hope it wasn't me!).

This is a major bug, as many scripts and the famous Rhino book (O'Reilly) all 
use the fact that regexp objects (including literals) are callable.

/be

Comment 11

[Phil Schwartau]

Will add test coverage for this -

Comment 12

[Mike McCabe]

r=mccabe.

I think I understand what's going on -

Debugging, I see 

          case JSOP_PUSHOBJ:
            PUSH_OPND(OBJECT_TO_JSVAL(obj));

where obj is null before the patch, JSVAL_TO_OBJECT(rval) without.  obj doesn't
seem to be assigned before this, so this could result in pushed garbage on
non-debug builds.  If I set it to a garbage value on entering the function, the
engine crashes.  obj is what we want on the stack - its value becomes thisp in
js_Invoke.  However, it looks like the engine discovers it by other means in
case obj is null.

Comment 13

[Brendan Eich [:brendan]]

Ah, you want obj = NULL, not obj = JSVAL_TO_OBJECT(rval).

The 'this' parameter of a regexp object literal when invoked (which runs its
exec method) should be the global object, I think.  This should match how ECMA
treats invocation, with this bound to the base of the reference type, if any and
if not an activation object, bound to null otherwise.

/be

Comment 14

[Brendan Eich [:brendan]]

Please review cvs diff -r3.1 -r3.2 -u jsinterp.c and restore other obj = NULL;
cases that were axed, if they seem necessary.  Let's have the full patch.  I'll
take a look myself at that diff, shortly.

/be

Comment 15

[rogerl (gone)]

Then I guess I don't understand the use of 'obj'. I thought it was a local copy 
of an object that is on top of the stack? If so, how can forcing it to NULL be 
right? (we don't know a call is coming up, do we?) If not, what are the rules 
for it's use?

Comment 16

[Brendan Eich [:brendan]]

obj is the "reference base" register for the last reference type computed in a 
callable expression.  When called, it becomes the nominal 'this' parameter, with 
the ECMA rules that activation => global and null => global.  It must be set 
carefully during property reference evaluations.  In the case of an object 
literal such as a regexp, it should be set to null (per ECMA, where there is no 
reference type, so GetBase or whatever it's called returns null).

/be

Comment 17

[rogerl (gone)]

In the diff you suggested, the other 'obj = NULL' removals I saw were 
for JSOP_UINT16 and JSOP_NULL. I guess in the case of 'null()', we would end up 
with garbage for a thisp, not a problem since js_invoke will bail before caring. 

Comment 18

[Brendan Eich [:brendan]]

Right: 5325() and null() both get stopped cold before thisp might be used.

/be

Comment 19

[Brendan Eich [:brendan]]

rogerl had to go, so I checked into the trunk for him.  Branch patch coming up
for mccabe to r= and me to a=.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: proposed Netscape6 branch fix

Comment 21

[Brendan Eich [:brendan]]

a=[e-mail] on that one-line branch patch.  McCabe, please give an r=
and restore [rtm+] instead of [rtm need info].

/be

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

r=shaver

Comment 23

[Brendan Eich [:brendan]]

There -- a crash bug with a one-line fix!  Please double-plus that rtm, Mr. 
Wizard!

/be

Comment 24

[Phil Peterson]

rtm++

Comment 25

[Brendan Eich [:brendan]]

Checked into branch (I'm taking no chances waiting till tomorrow).  Thanks to
everyone.

/be

Comment 26

[lchiang]

pschwartau: please verify when you get a chance. Thanks.

Comment 27

[Phil Schwartau]

Verified with trunk and MN6 builds of JS Engine 2000-11-05 on WinNT, Linux, Mac.