473709 - "Assertion failure: cursor == (uint8 *)copy->messageArgs[0] + argsCopySize, at jsexn.c" or "Assertion failure: cursor == (uint8 *)copy->messageArgs[0] + argsCopySize, at jsexn.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: linux gdb backtrace

function f() { eval("(function() { switch(x, x) { default: for(x2; <x><y/></x>;) (function(){}) <x><y/></x>;break; case (+<><x><y/></x></>): break;   }; })()"); }
gczeal(2);
f();

asserts debug js trunk shell (with and without -j) and 1.9.0.x debug js shell at Assertion failure: cursor == (uint8 *)copy->messageArgs[0] + argsCopySize, at ../jsexn.cpp:188

Compiling 1.9.0.x opt with gczeal seems to work as expected. Security-sensitive because it involves gczeal.

Thanks Waldo for guiding me along this one.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Easy fix. I gotta say, I like the ability to use stack-based helper classes in js/src now.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/d77738e770cd

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch backport

Here's the 1.9.0.x patch backport.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: diff between trunk and 1.9.0.x patch

To ensure nothing creepy sets in. :)

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 357288 [details] [diff] [review]
patch backport

Oops, something wonky in test compiles.

===

cc -o Darwin_DBG.OBJ/jsregexp.o -c -Wall -Wno-format -MMD -g3 -DXP_UNIX -DSVR4 -DSYSV -D_BSD_SOURCE -DPOSIX_SOURCE -DDARWIN -DX86_LINUX  -DDEBUG -DDEBUG_skywalker -DEDITLINE -IDarwin_DBG.OBJ  jsregexp.c
jsregexp.c: In function ‘js_NewRegExpObject’:
jsregexp.c:4310: error: ‘JSAutoTempValueRooter’ undeclared (first use in this function)
jsregexp.c:4310: error: (Each undeclared identifier is reported only once
jsregexp.c:4310: error: for each function it appears in.)
jsregexp.c:4310: error: syntax error before ‘tvr’
make[1]: *** [Darwin_DBG.OBJ/jsregexp.o] Error 1
make: *** [all] Error 2

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

jorendorff on IRC helped me out by telling me that mrbkap's patch "was changing from a C style of doing things to a C++ style of doing things" and suggested the following patch:

Index: jsregexp.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsregexp.c,v
retrieving revision 3.200
diff -u -8 -p -r3.200 jsregexp.c
--- jsregexp.c	11 Aug 2008 18:24:13 -0000	3.200
+++ jsregexp.c	16 Jan 2009 03:05:35 -0000
@@ -4303,20 +4303,20 @@ js_NewRegExpObject(JSContext *cx, JSToke
     JSString *str;
     JSObject *obj;
     JSRegExp *re;
     JSTempValueRooter tvr;
 
     str = js_NewStringCopyN(cx, chars, length);
     if (!str)
         return NULL;
+    JS_PUSH_TEMP_ROOT_STRING(cx, str, &tvr);
     re = js_NewRegExp(cx, ts,  str, flags, JS_FALSE);
     if (!re)
         return NULL;
-    JS_PUSH_TEMP_ROOT_STRING(cx, str, &tvr);
     obj = js_NewObject(cx, &js_RegExpClass, NULL, NULL, 0);
     if (!obj || !JS_SetPrivate(cx, obj, re)) {
         js_DestroyRegExp(cx, re);
         obj = NULL;
     }
     if (obj && !js_SetLastIndex(cx, obj, 0))
         obj = NULL;
     JS_POP_TEMP_ROOT(cx, &tvr);


However this causes the following assertion: Assertion failure: (cx)->tempValueRooters == (&pc->tempRoot), at jsparse.c:186 and a backtrace is:

(gdb) bt
#0  JS_Assert (s=0x10939c "(cx)->tempValueRooters == (&pc->tempRoot)", file=0x109340 "jsparse.c", ln=186) at jsutil.c:63
#1  0x000a5111 in js_FinishParseContext (cx=0x2005b0, pc=0xbfffdaa4) at jsparse.c:186
#2  0x000a61f8 in js_CompileScript (cx=0x2005b0, obj=0x17d100, principals=0x0, tcflags=2048, chars=0x202430, length=137, file=0x0, filename=0x201d81 "typein", lineno=1) at jsparse.c:671
#3  0x0008706f in obj_eval (cx=0x2005b0, obj=0x17d000, argc=1, argv=0x80609c, rval=0xbfffdcfc) at jsobj.c:1326
#4  0x0007b7ab in js_Invoke (cx=0x2005b0, argc=1, vp=0x806094, flags=2) at jsinterp.c:1304
#5  0x0006e6ca in js_Interpret (cx=0x2005b0) at jsinterp.c:4864
#6  0x0007c099 in js_Execute (cx=0x2005b0, chain=0x17d000, script=0x2023d0, down=0x0, flags=0, result=0xbffff628) at jsinterp.c:1546
#7  0x0001c910 in JS_ExecuteScript (cx=0x2005b0, obj=0x17d000, script=0x2023d0, rval=0xbffff628) at jsapi.c:4895
#8  0x00002c8b in Process (cx=0x2005b0, obj=0x17d000, filename=0x0, forceTTY=0) at js.c:310
#9  0x000035d8 in ProcessArgs (cx=0x2005b0, obj=0x17d000, argv=0xbffff760, argc=0) at js.c:556
#10 0x00009025 in main (argc=0, argv=0xbffff760, envp=0xbffff764) at js.c:3931

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> jorendorff on IRC helped me out by telling me that mrbkap's patch "was changing
> from a C style of doing things to a C++ style of doing things" and suggested
> the following patch:
> 
> Index: jsregexp.c
> ===================================================================
> RCS file: /cvsroot/mozilla/js/src/jsregexp.c,v
> retrieving revision 3.200
> diff -u -8 -p -r3.200 jsregexp.c
> --- jsregexp.c    11 Aug 2008 18:24:13 -0000    3.200
> +++ jsregexp.c    16 Jan 2009 03:05:35 -0000
> @@ -4303,20 +4303,20 @@ js_NewRegExpObject(JSContext *cx, JSToke
>      JSString *str;
>      JSObject *obj;
>      JSRegExp *re;
>      JSTempValueRooter tvr;
> 
>      str = js_NewStringCopyN(cx, chars, length);
>      if (!str)
>          return NULL;
> +    JS_PUSH_TEMP_ROOT_STRING(cx, str, &tvr);
>      re = js_NewRegExp(cx, ts,  str, flags, JS_FALSE);
>      if (!re)
>          return NULL;
> -    JS_PUSH_TEMP_ROOT_STRING(cx, str, &tvr);

Don't return without popping the tvr.

/be

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: working 1.9.0 patch

Thanks brendan and mrbkap, this is the next iteration of the patch, untested yet though.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 357302 [details] [diff] [review]
working 1.9.0 patch

Yep.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I have tested that Gary's patch works in my 1.9.0 tree.

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d77738e770cd

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-473709.js

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 357302 [details] [diff] [review]
working 1.9.0 patch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.9.0 branch.

Comment 15

[Bob Clary [:bc] (inactive)]

regressed by bug 443039

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Bob, was that comment really meant for this bug?

Comment 17

[Bob Clary [:bc] (inactive)]

mrbkap: yes. i just bisected it on x86_64 and thought i would comment it here.

Comment 18

[Daniel Veditz [:dveditz]]

qawanted: bc or gary, do we want or need this on the 1.8.1 branch?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Dan, yes we do.

Comment 20

[Samuel Sidler (old account; do not CC)]

Blake, can you work up a 1.8.1 patch?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the 1.8.1 branch

Comment 22

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c0779e4abe0a

Comment 23

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1, 1.9.2

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 one, based on the 1.8.1

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 360952 [details] [diff] [review]
Fix for the 1.8.1 branch

Approved for 1.8.1.21, a=dveditz for release-drivers.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.

Comment 27

[Alexander Sack]

Comment on attachment 363094 [details] [diff] [review]
1.8.0 one, based on the 1.8.1

a=asac for 1.8.0

Comment 28

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/547fc4916d3e
/cvsroot/mozilla/js/tests/e4x/Regress/regress-473709.js,v  <--  regress-473709.js
initial revision: 1.1