105707 - xpcshell crashes if fed 'line's longer than 4096 chars

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[timeless]

running on w98se
XPCSHELL EXE        12,944  10-09-01  5:45a xpcshell.exe

i'll attach the script and then the other stuff...

from Borland C++ 5.5 [free]'s Turbo Debugger

Exception: Access violation, Read address=4c474f64

 Last: 4294104489  Current: 4294104489  Total: 2  Notify: No
------------------------------------------------------------------------------
4294557157 (Thread 1)     ¦ Runnable, Priority=0(normal)
4294104489 (Thread 2)     ¦ Current location: :60D038AA
                          ¦ Exception: Access violation, Read address=4c474f64

+-[_]-CPU 80486 Thread #-862807---------------------------------------1-[][]-+
¦:60D03862 6A00           push   00000000                   eax 4C474F64 ¦c=0 ¦
¦:60D03864 FF7510         push   dword ptr [ebp+10]         _ebx 60D25588 ¦z=0 ¦
¦:60D03867 E81A800100     call   MSVCRT.strlen              _ecx 00761D00 ¦s=0 ¦
¦:60D0386C 8B7D08         mov    edi,[ebp+08]               _edx 001C36D8 ¦o=0 ¦
¦:60D0386F 59             pop    ecx                        _esi 00000000 ¦p=0 ¦
¦:60D03870 50             push   eax                        _edi 008024D0 ¦a=0 ¦
¦:60D03871 FF7510         push   dword ptr [ebp+10]         _ebp 0063EA64 ¦i=1 ¦
¦:60D03874 57             push   edi                        _esp 0063EA50 ¦d=0 ¦
¦:60D03875 E8503FFEFF     call   JS3250.js_Atomize          _  ds 016F    ¦    ¦
¦:60D0387A 83C410         add    esp,00000010               _  es 016F    ¦    ¦
¦:60D0387D 894510         mov    [ebp+10],eax               _  fs 272F    ¦    ¦
¦:60D03880 85C0           test   eax,eax                    _  gs 0000    ¦    ¦
¦:60D03882 7479           je     JS3250.60D038FD (60D038FD) _  ss 016F    ¦    ¦
¦:60D03884 8B450C         mov    eax,[ebp+0C]               _  cs 0167    ¦    ¦
¦:60D03887 BB8855D260     mov    ebx,60D25588               _eip 60D038AA ¦    ¦
¦:60D0388C 85C0           test   eax,eax                    _------------------¦
¦:60D0388E 751A           jne    JS3250.60D038AA (60D038AA) _:0063EA54 00000000¦
¦:60D03890 8B4734         mov    eax,[edi+34]               _:0063EA50008024D0¦
¦:60D03893 85C0           test   eax,eax                    _:0063EA4C 00000000¦
¦:60D03895 7407           je     JS3250.60D0389E (60D0389E) _:0063EA48 00000008¦
¦:60D03897 8B4034         mov    eax,[eax+34]               _:0063EA44 60D1C194¦
¦:60D0389A 85C0           test   eax,eax                    _:0063EA40 008024D0¦
¦:60D0389C 750C           jne    JS3250.60D038AA (60D038AA) _:0063EA3C 0000016F¦
¦:60D0389E 8BB78C000000   mov    esi,[edi+0000008C]         _:0063EA38 0063EA50¦
¦:60D038A4 85F6           test   esi,esi                    _:0063EA34 00010202¦
¦:60D038A6 7539           jne    JS3250.60D038E1 (60D038E1) _:0063EA30 00000167¦
¦:60D038A8 EB5E           jmp    JS3250.60D03908 (60D03908) _:0063EA2C 60D038AA¦
¦:60D038AA8B08           mov    ecx,[eax]                  _:0063EA28 0063EA64¦
¦:60D038AC 8BF0           mov    esi,eax                    :0063EA24 4C474F64¦
¦_________________________________________________________¦:0063EA20 00761D00¦
¦:00000000 9E 0F C9 00 65 04 70 00 P¤+ ep                  ¦:0063EA1C 001C36D8¦
¦:00000008 16 00 60 05 65 04 70 00  `ep                  ¦:0063EA18 60D25588¦
¦:00000010 65 04 70 00 54 FF 00 F0 ep T_ _                 ¦:0063EA14 00000000¦
¦:00000018 38 7F 00 F0 13 E8 00 F0 8? __ _                 ¦:0063EA10 008024D0¦
¦:00000020 00 00 00 CD 28 00 60 05    -( `                 ¦:0063EA0C 0000016F¦
¦:00000028 6F EF 00 F0 6F EF 00 F0 o_ _o_ _                 ¦:0063EA08 0000016F¦
¦:00000030 6F EF 00 F0 6F EF 00 F0 o_ _o_ _                 ¦:0063EA04 0000272F¦
¦:00000038 9A 00 60 05 65 04 70 00 Ü `ep                  ¦:0063EA00 00000000¦
¦:00000040 07 00 70 CD 4D F8 00 F0  p-M° _                 ¦:0063E9FC 0000000A¦
+------------------------------------------------------------------------------+

I can't figure out how to get a stack trace :-(

Comment 1

[timeless]

Attachment: load file into notepad, select all, copy, run xpcshell in a window, paste, move mouse alot (paste on 9x sucks) until xpshell crashes

Comment 2

[timeless]

Last pasted lines:
for (foSmb8UqOw=almoOdOGLaS.length-1;foSmb8UqOw>=0;foSmb8UqOw--) {
fo3Mb8uq0w+=almoOdOGLaS.charAt(foSmb8UqOw);

Comment 3

[timeless]

Attachment: dr watson log

Comment 4

[timeless]

Attachment: the email containing the javascript

Comment 5

[Phil Schwartau]

I was able to duplicate this crash in the SpiderMonkey shell on WinNT,
by following the exact steps-to-reproduce at attachment 54240 [details] above.

The stack trace, however, only consisted of this:

strlen() line 66
Process(JSContext * 0x0012ffe0, JSObject * 0x00403f62 _except_handler3,
        char * 0x00404228) line 355 + 9 bytes
0012ecf0()



If I load attachment 54240 [details] as a file, however, I do not crash:
[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js -f 54240.js

Or: 

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> load('54240.js');


Both of these work fine - 

Comment 6

[Phil Schwartau]

Reassigning to Kenton; cc'ing others for advice - is this a JS Engine bug?

Comment 7

[Phil Schwartau]

Attachment: Crashpoint in strlen function at case str_misaligned:

Comment 8

[Phil Schwartau]

Note: on my WinNT box, the strlen function was accessed from

  C:\Program Files\Microsoft Visual Studio\VC98\CRT\SRC\Intel\Strlen.asm

Comment 9

[John Bandhauer]

It is crasing because the function 'Process' (in xpcshell.c and js.c) has a 
fixed 'line' buffer size of 4096 chars and this file is much bigger than that.

I'm marking this WONTFIX and saying "Don't do that!" Pasting big multiline gobs 
into the shells is not supported. Save as a text file and pass on the command 
line or use load('foo.js').

Comment 10

[Phil Schwartau]

Thank you; marking verified -

Comment 11

[timeless]

this is going to keep biting me, and since i'm the consumer...

Comment 12

[timeless]

... i'll provide the fix.

Comment 13

[timeless]

Attachment: temporarily grow buffer by doubling

There are a few things I'd imagine reviewers might not like (beyond the fact
that i reopened this bug).
1. GetLine returns JS_FALSE instead of 0. I'll certainly change that if people
have an opinion
2. I picked int instead of unsigned int.
3. I don't hold onto the grown buffer for the duration of the session. I
certainly could but at least for the way I'm using xpcshell i don't need to.
4. I added JS_ASSERTs to a file which currently couldn't compile w/ the
JS_ASSERTs already in the file.

Note that I'm running xpcshell on 3-5 boxes these days, some using release
versions some using cvs builds. I'm building arrays of strings and I don't want
to be forced to use load().

Comment 14

[timeless]

Comment on attachment 108625 [details] [diff] [review]
temporarily grow buffer by doubling

my whitespace is a bit inconsistent -- sorry.

dbradley/brendan would you please consider this patch?

Comment 15

[John Bandhauer]

*If* this is worth changing in xpcshell.cpp then it is worth changing in js.c
also. IMO, the more these two shells stay alike the better.

Comment 16

[timeless]

Attachment: equivalent changes for js.c

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 108627 [details] [diff] [review]
equivalent changes for js.c

>@@ -249,8 +249,9 @@
> extern void     add_history(char *line);
> #endif
> 
>-static JSBool
>+static int
> GetLine(JSContext *cx, char *bufp, FILE *file, const char *prompt) {

Someone was on Java drugs -- could you please fix the { to be at the start of
its own line?  Thanks.

>+    int len;
> #ifdef EDITLINE
>     /*
>      * Use readline only if file is stdin, because there's no way to specify
>@@ -264,7 +265,8 @@
>             add_history(linep);
>         strcpy(bufp, linep);
>         JS_free(cx, linep);
>-        bufp += strlen(bufp);
>+        len = strlen(bufp);
>+        bufp += len++;
>         *bufp++ = '\n';

How do we know there's room at bufp before the strcpy or the '\n' (or the '\0'
for that matter)?

>         *bufp = '\0';
>     } else
>@@ -279,8 +281,9 @@
>         if (fgets(line, sizeof line, file) == NULL)
>             return JS_FALSE;

Shouldn't that be return 0 in the new version?

>         strcpy(bufp, line);
>+        len = strlen(line);
>     }
>-    return JS_TRUE;
>+    return len;
> }
> 
> static void
>@@ -290,11 +293,17 @@
>     JSScript *script;
>     jsval result;
>     JSString *str;
>-    char buffer[4096];
>-    char *bufp;
>+    int linelen;
>     int lineno;
>     int startline;
>     FILE *file;
>+    char basebuffer[4098];

4098?

>+    int bufsize;
>+    int buflen;
>+    char *buffer;
>+    char *bufp;
>+    int newsize;
>+    char *newbuf;

You can combine these declarations if they share the same (pointer-to) type.

> 
>     if (filename != NULL && strcmp(filename, "-") != 0) {

Pre-existing code, but feel free to change foo != NULL to test just foo.

>         file = fopen(filename, "r");
>@@ -337,7 +346,9 @@
>     lineno = 1;
>     hitEOF = JS_FALSE;
>     do {
>-        bufp = buffer;
>+        bufsize = sizeof(basebuffer);
>+        buflen = 0;
>+        bufp = buffer = basebuffer;
>         *bufp = '\0';
> 
>         /*
>@@ -348,17 +359,32 @@
>          */
>         startline = lineno;
>         do {
>-            if (!GetLine(cx, bufp, file, startline == lineno ? "js> " : "")) {
>+            if (buflen + 256 >= bufsize) {

Why the magic 256?  If we're using the GNU readline/editline stuff, is that a
well-known constant that readline uses?  If so, is there no #define to use,
here and above in GetLine?

>+                newsize = bufsize * 2;
>+                JS_ASSERT(newsize > bufsize);
>+                newbuf = (char*)malloc(newsize);

Spaces, please -- let the cast and the * in it breathe a bit.

>+                JS_ASSERT(newbuf);

Other places in the shell cope with OOM, why not this one?

/be

Comment 18

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 19

[Adam Guthrie]

*** Bug 347967 has been marked as a duplicate of this bug. ***

Comment 20

[Robert Kaiser]

Is this still relevant? If so, should we drive the patches in? Do we have crash signatures?
If it's not relevant any more, can we close it out?

Comment 21

[Josh Matthews [:jdm]]

Possibly still relevant, but you won't see crash signatures since this is the test harness, not the browser.

Comment 22

[Daniel Veditz [:dveditz]]

bug 824394 reports the same bug in /ipc/testshell/XPCShellEnvironment.cpp which appears to have been based on xpcshell.