336409 - probably an integer overflow in js_obj_toSource

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

there is probably an integer overflow in js_obj_toSource  :
var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9: r};
var rr=o.toSource();

where r occupies 512MB

causes this crash:
#4  0xb740d7b6 in nanosleep () from /lib/tls/libc.so.6
#5  0xb740d5df in sleep () from /lib/tls/libc.so.6
#6  0xb7f4a96b in ah_crap_handler (signum=11) at nsSigHandlers.cpp:133
#7  0xb7f62cc4 in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:210
#8  <signal handler called>
#9  0xb7e6aba5 in QuoteString (sp=0xbf9c09c8, str=0x88fdde0, quote=34)
    at /opt/joro/firefox/mozilla/js/src/jsopcode.c:438
#10 0xb7e6ad6c in js_QuoteString (cx=0x86a89e0, str=0x88fdde0, quote=34)
    at /opt/joro/firefox/mozilla/js/src/jsopcode.c:496
#11 0xb7e9d5d2 in js_ValueToSource (cx=0x86a89e0, v=143646180)
    at /opt/joro/firefox/mozilla/js/src/jsstr.c:2697
#12 0xb7e5e3d2 in js_obj_toSource (cx=0x86a89e0, obj=0x88fdde8, argc=0, 
    argv=0x814f814, rval=0xbf9c0c28)
    at /opt/joro/firefox/mozilla/js/src/jsobj.c:869
#13 0xb7e3700a in js_Invoke (cx=0x86a89e0, argc=0, flags=0)
    at /opt/joro/firefox/mozilla/js/src/jsinterp.c:1300
#14 0xb7e4ac0f in js_Interpret (cx=0x86a89e0, pc=0x8151c5a ":", 
    result=0xbf9c1824) at /opt/joro/firefox/mozilla/js/src/jsinterp.c:3955

memory usage is about 600MB and probably can be made smaller.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: crash

Comment 2

[Daniel Veditz [:dveditz]]

Is this x86_64 Linux running 32-bit Firefox as in bug 335535? Asking in case it's relevant. Don't see a problem (beyond the temporary busy-script freeze) in 32-bit windows.

Comment 3

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> Is this x86_64 Linux running 32-bit Firefox as in bug 335535? Asking in case
> it's relevant. Don't see a problem (beyond the temporary busy-script freeze) in
> 32-bit windows.
> 

according to me this is pure 32 bit issue. i crash on 32 bit linux and 32 bit macosx ppc with the same stack.

do you get out of memory error?

Comment 4

[georgi - hopefully not receiving bugspam]

this issue is similar to Bug 336410

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I'll look at this tomorrow.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-336409-1.js (96M)

This test occasionally gave me the following stack (as did 64M).

=>         return HeapAlloc(_crtheap, 0, size ? size : 1);

		__active_heap	0x00000001	int
		_crtheap	0x00af0000	void *
		size	0x00000238	unsigned int

 	ntdll.dll!_RtlAllocateHeap@12()  + 0xe5a bytes	
>	msvcr80d.dll!_heap_alloc_base(unsigned int size=0x00000238)  Line 105 + 0x28 bytes	C
 	msvcr80d.dll!_heap_alloc_dbg(unsigned int nSize=0x00000214, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 409 + 0x9 bytes	C++
 	msvcr80d.dll!_nh_malloc_dbg(unsigned int nSize=0x00000214, int nhFlag=0x00000000, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 266 + 0x15 bytes	C++
 	msvcr80d.dll!_malloc_dbg(unsigned int nSize=0x00000214, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 189 + 0x1b bytes	C++
 	msvcr80d.dll!_calloc_dbg(unsigned int nNum=0x00000001, unsigned int nSize=0x00000214, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 561 + 0x15 bytes	C++
 	msvcr80d.dll!__CRTDLL_INIT(void * hDllHandle=0x10200000, unsigned long dwReason=0x00000002, void * lpreserved=0x00000000)  Line 434 + 0x18 bytes	C
 	msvcr80d.dll!_CRTDLL_INIT(void * hDllHandle=0x10200000, unsigned long dwReason=0x00000002, void * lpreserved=0x00000000)  Line 214 + 0x11 bytes	C
 	ntdll.dll!_LdrpCallInitRoutine@16()  + 0x14 bytes	
 	ntdll.dll!_LdrpInitializeThread@4()  + 0xcb bytes	
 	ntdll.dll!__LdrpInitialize@12()  + 0x78 bytes	
 	ntdll.dll!_KiUserApcDispatcher@20()  + 0x7 bytes

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-336409-2.js (128M)

This test originally replicated the reported stack.

Comment 8

[georgi - hopefully not receiving bugspam]

i suspect the 96MB attachment is with little misleading wording:

function createString(n)
{
  var l = n*1024*1024;
  var r = 'r';

  while (r.length < l)
  {
    r = r + r;
^^^^^^^^^^^^^^^ here length can be only a power of 2. so when it is >= l it is a power of two.
  }
  return r;
}

var n = 96;
printStatus('Creating ' + n + 'MB string');
^^^^^^ this is a little misleading wording.
var r = createString(n);
printStatus('Done. length = ' + r.length);
^^^^^ check this:
*****alert('Done. length = ' + r.length+ " expected="+n*1024*1024);

Comment 9

[georgi - hopefully not receiving bugspam]

also length does not mean bytes (or megabytes).

2*length is the byte size at least on x86.

Comment 10

[Bob Clary [:bc] (inactive)]

changed the 96 to 64 and MB to M length here and in 336410.

Comment 11

[Jay Patel [:jay]]

Blake:  Did you get a chance to look at this?  Let's try to get a handle on this quickly.

Comment 12

[Jay Patel [:jay]]

mrbkap:  We are approaching code freeze for 1.5.0.5, and need to get a patch soon if it's gonna make the release.  Please give us an update on your progress.  Thanks!

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Something like this

This is pretty ugly, but I think it handles all possible overflows.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 226414 [details] [diff] [review]
Something like this

>+#define SAFE_ADD(n)                                                          \
>+            JS_BEGIN_MACRO                                                   \
>+                curlen += (n);                                               \
>+                if (curlen < (n))                                            \
>+                    goto overflow;                                           \
>+            JS_END_MACRO

Nit: it doesn't look better to my eyes to indent the body this way, esp. since the guts in the JS_BEGIN/END_MACRO are indented one more level.

More significantly given what follows, (n) should be assigned to a local n_ or similar, to avoid lame compilers failing to common the repeated (n) in the macro body.

Or, reassure me that GCC and MSVC do common, and I'll worry less.  Still seems a style break from the rest of SpiderMonkey.

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Pick nits

Brendan, is this what you had in mind?

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 226453 [details] [diff] [review]
Pick nits

Sure.

Should we use something like this anywhere else?

/be

Comment 18

[georgi - hopefully not receiving bugspam]

+    JS_BEGIN_MACRO                                                           \
+        size_t n_ = (n);                                                     \
+        curlen += n_;                                                        \
+        if (curlen < n_)                                                     \
+            goto overflow;                                                   \
+    JS_END_MACRO

i am not sure this is effective for n == (size_t)-1

Comment 19

[georgi - hopefully not receiving bugspam]

(In reply to comment #18)
> i am not sure this is effective for n == (size_t)-1
> 

oops, this was dumb, never mind.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 226414 [details] [diff] [review]
Something like this

approved for 1.8.0 branch, a=dveditz for drivers

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 226453 [details] [diff] [review]
Pick nits

approved for 1.8.0 branch, a=dveditz for drivers

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch.

Comment 23

[Bob Clary [:bc] (inactive)]

I am getting intermittent crashes in 1.5.0.5/all platforms, I doubt it is the same bug but I can't verify the fix.

On WinXp I get varying stacks:

>	xpcom_core.dll!TimerThread::Run()  Line 252 + 0x3 bytes	C++
 	xpcom_core.dll!nsThread::Main(void * arg=0x013ff970)  Line 118 + 0x1a bytes	C++
 	nspr4.dll!_PR_NativeRunThread(void * arg=0x013fe430)  Line 436 + 0xd bytes	
 	[Frames below may be incorrect and/or missing, no symbols loaded for nspr4.dll]	
 	nspr4.dll!pr_root(void * arg=0x013fe430)  Line 112 + 0xd bytes	
 	MSVCRTD.DLL!_threadstartex(void * ptd=0x013fe660)  Line 212 + 0xd bytes	C
 	kernel32.dll!_BaseThreadStart@8()  + 0x37 bytes	

and 

>	js3250.dll!QuoteString(Sprinter * sp=0x0012e998, JSString * str=0x02b45df8, unsigned short quote=0x0022)  Line 459 + 0x8 bytes	C
 	js3250.dll!js_QuoteString(JSContext * cx=0x03111c98, JSString * str=0x02b45df8, unsigned short quote=0x0022)  Line 497 + 0x12 bytes	C
 	js3250.dll!js_ValueToSource(JSContext * cx=0x03111c98, long v=0x02b45dfc)  Line 2792 + 0x12 bytes	C
 	js3250.dll!js_obj_toSource(JSContext * cx=0x03111c98, JSObject * obj=0x02b45ea0, unsigned int argc=0x00000000, long * argv=0x033bde10, long * rval=0x0012eb80)  Line 898 + 0x13 bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x03111c98, unsigned int argc=0x00000000, unsigned int flags=0x00000000)  Line 1188 + 0x17 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x03111c98, unsigned char * pc=0x033d6676, long * result=0x0012f5e0)  Line 3583 + 0xf bytes	C
 	js3250.dll!js_Execute(JSContext * cx=0x03111c98, JSObject * chain=0x02b16e70, JSScript * script=0x033d6548, JSStackFrame * down=0x00000000, unsigned int flags=0x00000000, long * result=0x0012f6e8)  Line 1434 + 0x13 bytes	C
 	js3250.dll!JS_EvaluateUCScriptForPrincipals(JSContext * cx=0x03111c98, JSObject * obj=0x02b16e70, JSPrincipals * principals=0x0100d2b4, const unsigned short * chars=0x033b9ad8, unsigned int length=0x000009d6, const char * filename=0x033cce58, unsigned int lineno=0x00000001, long * rval=0x0012f6e8)  Line 4122 + 0x19 bytes	C
 	gklayout.dll!nsJSContext::EvaluateString(const nsAString_internal & aScript={...}, void * aScopeObject=0x02b16e70, nsIPrincipal * aPrincipal=0x0100d2b0, const char * aURL=0x033cce58, unsigned int aLineNo=0x00000001, const char * aVersion=0x100e0844, nsAString_internal * aRetValue=0x00000000, int * aIsUndefined=0x0012f74c)  Line 1061 + 0x43 bytes	C++
 	gklayout.dll!nsScriptLoader::EvaluateScript(nsScriptLoadRequest * aRequest=0x033cd998, const nsString & aScript={...})  Line 774	C++
 	gklayout.dll!nsScriptLoader::ProcessRequest(nsScriptLoadRequest * aRequest=0x033cd998)  Line 672 + 0x16 bytes	C++
 	gklayout.dll!nsScriptLoader::OnStreamComplete(nsIStreamLoader * aLoader=0x033b3720, nsISupports * aContext=0x033cd998, unsigned int aStatus=0x00000000, unsigned int stringLen=0x000009d6, const unsigned char * string=0x033cf4e8)  Line 1039	C++
 	necko.dll!nsStreamLoader::OnStopRequest(nsIRequest * request=0x033ccf28, nsISupports * ctxt=0x033cd998, unsigned int aStatus=0x00000000)  Line 137	C++
 	necko.dll!nsStreamListenerTee::OnStopRequest(nsIRequest * request=0x033ccf28, nsISupports * context=0x033cd998, unsigned int status=0x00000000)  Line 66	C++
 	necko.dll!nsHttpChannel::OnStopRequest(nsIRequest * request=0x033cf388, nsISupports * ctxt=0x00000000, unsigned int status=0x00000000)  Line 4053	C++
 	necko.dll!nsInputStreamPump::OnStateStop()  Line 507	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x033c5680)  Line 343 + 0xb bytes	C++
 	xpcom_core.dll!nsInputStreamReadyEvent::EventHandler(PLEvent * plevent=0x033cf47c)  Line 120	C++
 	xpcom_core.dll!PL_HandleEvent(PLEvent * self=0x033cf47c)  Line 688 + 0xa bytes	C
 	xpcom_core.dll!PL_ProcessPendingEvents(PLEventQueue * self=0x00f92368)  Line 623 + 0x9 bytes	C
 	xpcom_core.dll!_md_EventReceiverProc(HWND__ * hwnd=0x0045031c, unsigned int uMsg=0x0000c143, unsigned int wParam=0x00000000, long lParam=0x00f92368)  Line 1408 + 0x9 bytes	C
 	user32.dll!77d48734() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	

Comment 24

[Bob Clary [:bc] (inactive)]

comment 23 was for 1.5.0.5/browser for js1_5/Regress/regress-336409-1.js.

I also get crashes in trunk/browser on windows and mac for js1_5/Regress/regress-336409-2.js with stacks like 

 	ntdll.dll!_RtlAllocateHeap@12()  + 0xe5a bytes	
>	msvcr80d.dll!_heap_alloc_base(unsigned int size=0x00000238)  Line 105 + 0x28 bytes	C
 	msvcr80d.dll!_heap_alloc_dbg(unsigned int nSize=0x00000214, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 409 + 0x9 bytes	C++
 	msvcr80d.dll!_nh_malloc_dbg(unsigned int nSize=0x00000214, int nhFlag=0x00000000, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 266 + 0x15 bytes	C++
 	msvcr80d.dll!_malloc_dbg(unsigned int nSize=0x00000214, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 189 + 0x1b bytes	C++
 	msvcr80d.dll!_calloc_dbg(unsigned int nNum=0x00000001, unsigned int nSize=0x00000214, int nBlockUse=0x00000002, const char * szFileName=0x102ccf50, int nLine=0x000001b2)  Line 561 + 0x15 bytes	C++
 	msvcr80d.dll!__CRTDLL_INIT(void * hDllHandle=0x10200000, unsigned long dwReason=0x00000002, void * lpreserved=0x00000000)  Line 434 + 0x18 bytes	C
 	msvcr80d.dll!_CRTDLL_INIT(void * hDllHandle=0x10200000, unsigned long dwReason=0x00000002, void * lpreserved=0x00000000)  Line 214 + 0x11 bytes	C
 	ntdll.dll!_LdrpCallInitRoutine@16()  + 0x14 bytes	
 	ntdll.dll!_LdrpInitializeThread@4()  + 0xcb bytes	
 	ntdll.dll!__LdrpInitialize@12()  + 0x78 bytes	
 	ntdll.dll!_KiUserApcDispatcher@20()  + 0x7 bytes	

I can't verify on the trunk either.

Comment 25

[georgi - hopefully not receiving bugspam]

(In reply to comment #23)
> I am getting intermittent crashes in 1.5.0.5/all platforms, I doubt it is the
> same bug but I can't verify the fix.
> 

i am getting crashes on linux and macosx ppc with the attachment "crash":

debug linux trunk: gdb fires with the same stack in the description
linux 1.5.0.5rc1 nondebug: xterm says "glibc detected 'heap screw'", nonresponsive firefox, attaching with gdb shows glibc has aborted() and raised()
macosx ppc 1.5.0.5rc1 nondebug: firefox closes, but a rainbow spinning wheel cursor remains. macosx seems unusable.

Comment 26

[georgi - hopefully not receiving bugspam]

this killed macosx ppc 3 consecutive times with 1.5.0.5rc1.

bob, does your macosx survive?

Comment 27

[Bob Clary [:bc] (inactive)]

(In reply to comment #26)
> this killed macosx ppc 3 consecutive times with 1.5.0.5rc1.
> 
> bob, does your macosx survive?
> 

I have been running opt and debug builds and all of my mac machine survived the latest round of test this weekend although one unexpectedly and for no reason keeled over and died last night.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

It sounds like my patch didn't really fix this.

Comment 29

[Daniel Veditz [:dveditz]]

Removing the fixed1.8.0.5 keyword so this stays on the radar whil Blake looks into this.

Comment 30

[georgi - hopefully not receiving bugspam]

after some debugging, i suspect that the current problem lies in QuoteString and probably the arena stuff.

from gdb on trunk:

at attachment "crash" at the alert "done obj":
(gdb) break jsobj.c:910
Breakpoint 1 at 0xb7e5fb59: file /opt/joro/firefox/mozilla/js/src/jsobj.c, line 910.
(gdb) cont
Continuing.
[Thread -1263719504 (LWP 7029) exited]

Breakpoint 1, js_obj_toSource (cx=0x88ca450, obj=0x8907a68, argc=0, 
    argv=0x8c36c24, rval=0xbf9c79c4)
    at /opt/joro/firefox/mozilla/js/src/jsobj.c:910
910                 valstr = js_ValueToSource(cx, val[j]);
Current language:  auto; currently c
(gdb) break jsopcode.c:472
Breakpoint 2 at 0xb7e6d983: file /opt/joro/firefox/mozilla/js/src/jsopcode.c, line 472.
(gdb) cont
Continuing.

Breakpoint 2, QuoteString (sp=0xbf9c7758, str=0x8907a70, quote=34)
    at /opt/joro/firefox/mozilla/js/src/jsopcode.c:472
472             len = PTRDIFF(t, s, jschar);
(gdb) next
475             nb = (sp->offset + len + 1) - sp->size;
(gdb) next
476             if (nb > 0 && !SprintAlloc(sp, nb))
(gdb) next
480             bp = sp->base + sp->offset;
(gdb) next
481             sp->offset += len;
(gdb) next
482             while (--len >= 0)
(gdb) li
477                 return NULL;
478     
479             /* Advance sp->offset and copy s into sp's buffer. */
480             bp = sp->base + sp->offset;
481             sp->offset += len;
482             while (--len >= 0)
483                 *bp++ = (char) *s++;
484             *bp = '\0';
485     
486             if (t == z)

(gdb) p bp
$1 = 0x8c4f599 ""
(gdb) p bp + len
$2 = 0x18c4f599 <Address 0x18c4f599 out of bounds>
(gdb) p bp + len - 10000
$3 = 0x18c4ce89 <Address 0x18c4ce89 out of bounds>
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0xb7e6da07 in QuoteString (sp=0xbf9c7758, str=0x8907a70, quote=34)
    at /opt/joro/firefox/mozilla/js/src/jsopcode.c:483
483                 *bp++ = (char) *s++;
(gdb) info locals
off = 0
len = 268121496
nb = 268435456
s = (const jschar *) 0x727664d6
t = (const jschar *) 0x926cd008
u = (const jschar *) 0x8907a68
z = (const jschar *) 0x926cd008
bp = 0x8c9c000 <Address 0x8c9c000 out of bounds>
c = 0
ok = 145798020

(gdb) x/4x bp-200
0x8c9bf38:      0x76767676      0x76767676      0x76767676      0x76767676
(gdb)

Comment 31

[georgi - hopefully not receiving bugspam]

String.toSource() crashes with similar top stack on trunk & 1.5.0.4 & 1.5.0.5
probably due to arena, though not sure at all.

testcase to follow.

should i file new bug about String.toSource()?

Comment 32

[georgi - hopefully not receiving bugspam]

Attachment: String.toSource() crash

Comment 33

[georgi - hopefully not receiving bugspam]

String.toSource is Bug 342960

Comment 34

[Daniel Veditz [:dveditz]]

Back to fixed, Blake says the remaining crash is actually bug 342960.

Comment 35

[georgi - hopefully not receiving bugspam]

is it possible this bug to be invalid and all problems to have been caused by the arena?

Comment 36

[Bob Clary [:bc] (inactive)]

I can't reproduce it on windows locally but I am showing crashes in browser tests for 1.8.0.5, 1.8.1a3 and 1.9a1 on windows/macppc/linux. Note that the String.toSource() bug is verified fixed on 1.8.0.5/1.9a1 but not on 1.8.1a3 so if these crashes on 1.8.0.5 and 1.9a1 are real then they are _not bug 342960_. I'll know more hopefully with today's builds. leaving fixed status alone for now.

Comment 37

[Bob Clary [:bc] (inactive)]

browser based js1_5/Regress/regress-336409-1.js, js1_5/Regress/regress-336409-2.js crashes on linux 20060707 builds for 1.8.0.5, 1.8.1 and 1.9. Since bug 342960 appears to be fixed for 1.8.0.5 and 1.9, it is not the cause of the crashes here.

reopen and removed the fixed* keywords until someone tells me otherwise.

Comment 38

[georgi - hopefully not receiving bugspam]

on today linux trunk i don't crash with my testcase after 8 reloads.

valgrind seems unusable in this case because it takes at least 4 hours.

Comment 39

[Bob Clary [:bc] (inactive)]

per discussion with jay, marcia and dveditz: marking fixed and readding the fixed1.8.0.5 and fixed1.8.1 keywords.

Comment 40

[Bob Clary [:bc] (inactive)]

filed Bug 344137

Comment 41

[Alexander Sack]

Attachment: 1.0.x all in one patch

Comment 42

[Bob Clary [:bc] (inactive)]

1.8, 1.9 windows, mac* do not crash on either testcase. 1.8, 1.9 linux all exit on the test farm with SIGABRT, but I can't reproduce it locally or on the qa farm machines. Whatever the situation it is not this bug, so I will verify.

verify fixed 1.8, 1.9

Comment 43

[georgi - hopefully not receiving bugspam]

don't crash/exit on linux: trunk, 1.5-latest, 2.0-latest

get out of memory

Comment 44

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-336409-1.js

modify expected exit code for the shell per bug 358975.

Comment 45

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-336409-2.js

modify expected exit code for the shell per bug 358975.

Comment 46

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-336409-1.js,v  <--  regress-336409-1.js

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-336409-2.js,v  <--  regress-336409-2.js

moved to extensions/ since it uses toSource