342960 - String.toSource() on a large string causes crash, probably due to arena bug

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

String.toSource() on a large string causes crash, probably due to arena bug

483                 *bp++ = (char) *s++;
Current language:  auto; currently c
(gdb) x/4x bp
0x8ca6000:       Cannot access memory at address 0x8ca6000

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: String.toSource()

Comment 2

[Bob Clary [:bc] (inactive)]

Doesn't crash recent trunk on winxp. Georgi: any reason you didn't file this under JavaScript Engine?

Comment 3

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> Doesn't crash recent trunk on winxp. Georgi: any reason you didn't file this
> under JavaScript Engine?
> 

what about linux and mac?

Comment 4

[Bob Clary [:bc] (inactive)]

It crashes on linux trunk after clicking the "done" alert and on a second try with winxp trunk it also crashes with an invalid allocation size.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

So, this is almost the same bug as bug 338121, except in the growing case as opposed to the allocating case. With this patch, the testcase runs and ends up reporting out of memory (unsurprisingly, really).

Comment 6

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> With this patch, the testcase runs and ends up
> reporting out of memory (unsurprisingly, really).
> 

out of memory kinda surprises me. before toSource() firefox uses 613M memory and after it should use about 512M more, which is not that much virtual memory.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 227486 [details] [diff] [review]
Fix

approved for 1.8.0 branch pending brendan's review, a=dveditz for drivers

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 227486 [details] [diff] [review]
Fix

Looks good -- we just read through JS_ArenaRealloc and JS_ArenaGrow and believe they are safe (the former because realloc will fail on any overlarge gross size, the latter because it's layered on JS_ARENA_ALLOCATE).

/be

Comment 9

[georgi - hopefully not receiving bugspam]

-            if ((jsuword)(p) <= _a->limit - _nb) {                            \
+            if (_a->limit >= _nb && (jsuword)(p) <= _a->limit - _nb) {       \
                 _a->avail = (jsuword)(p) + _nb;                               \
                 JS_ArenaCountInplaceGrowth(pool, size, incr);                 \

is the second backslash misaligned on purpose?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

> is the second backslash misaligned on purpose?

Oops, no it isn't. I'll fix it before checking in.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch and trunk.

Comment 12

[Mike Schroepfer]

Comment on attachment 227486 [details] [diff] [review]
Fix

Approving for branch since this is on trunk and 1.8.0 branch.

Comment 13

[Bob Clary [:bc] (inactive)]

This appears to be fixed in the shell on 1.8.0.5 and trunk, but I still get crashes in the browser on 1.8.0.5 and trunk on linux for  js1_5/Regress/regress-336409-[12].js

Comment 14

[Bob Clary [:bc] (inactive)]

ditto js1_5/Regress/regress-336410-[12].js

Comment 15

[georgi - hopefully not receiving bugspam]

on linux trunk i get sporadic crashes on reloading String.toSource() several times.

Assertion failure: offsetInArena < GC_THINGS_SIZE, at /opt/joro/firefox/mozilla/js/src/jsgc.c:502

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-342960.js

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.5, 1.9a1 windows/macppc/linux.

not fixed 1.8.1a3
crashes shell windows, browser windows/macppc/linux

Comment 18

[Bob Clary [:bc] (inactive)]

with 20060707 builds, js1_5/GC/regress-342960.js browser tests no longer crashes windows/macppc 1.8.1 but _does crash linux 1.8.1_.

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #15)
> on linux trunk i get sporadic crashes on reloading String.toSource() several
> times.
> 
> Assertion failure: offsetInArena < GC_THINGS_SIZE, at
> /opt/joro/firefox/mozilla/js/src/jsgc.c:502

Cc'ing Igor.

(In reply to comment #18)
> with 20060707 builds, js1_5/GC/regress-342960.js browser tests no longer
> crashes windows/macppc 1.8.1 but _does crash linux 1.8.1_.

Between these two, I think we need a new bug or two.  It's important to cite the crash stack or any other skidmarks, though, so we can know whether to file one bug (for the assertion) or two.  Start with one.

/be

Comment 20

[Igor Bukanov]

Comment on attachment 228295 [details]
js1_5/GC/regress-342960.js

>		writeLineToLog("don't interrupt the script. let it go.");
>		for(i=0;i<1024*1024;i++) meg += "v";	
>		for(i=0;i<1024/4;i++) r += meg;
>		var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9: r};
>		writeLineToLog('done obj');
>		var rr=r.toSource();
>		writeLineToLog('done toSource()');

Is the line "var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9: r};" really necessary to trigger the crash on 1.8.1? What happens without it? Or was the test case supposed to use o.toSource() and not r.toSource()?

Comment 21

[georgi - hopefully not receiving bugspam]

> 
> Is the line "var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9:
> r};" really necessary to trigger the crash on 1.8.1? What happens without it?
> Or was the test case supposed to use o.toSource() and not r.toSource()?
> 

in my testcase it was left from the previous bug.

String.toSource() crashed without this line locally, so i believe it is not necessary for this bug, though it may trigger something.

Comment 22

[georgi - hopefully not receiving bugspam]

(In reply to comment #20)

> Or was the test case supposed to use o.toSource() and not r.toSource()?
> 

o.toSource() is different bug (seems fixed).
|o| was just left from the above bug.

Comment 23

[Bob Clary [:bc] (inactive)]

mrbkap: can you please land this on 1.8 today? 

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, this was checked in with js1.7.

Comment 25

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 win/mac(ppc|tel)/linux 20060725

Comment 26

[Alexander Sack]

Attachment: 1.0.x version of the patch

Comment 27

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/GC/regress-342960.js

modify expected exit code for the shell per bug 358975.

Comment 28

[Bob Clary [:bc] (inactive)]

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