The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: mrbkap)

Tracking

({verified1.8.0.5, verified1.8.1})

Trunk
mozilla1.9alpha1
verified1.8.0.5, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(4 attachments, 1 obsolete attachment)

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
Created attachment 227392 [details]
String.toSource()

Comment 2

11 years ago
Doesn't crash recent trunk on winxp. Georgi: any reason you didn't file this under JavaScript Engine?
(Reporter)

Updated

11 years ago
Component: General → General
Product: Firefox → Core
(Reporter)

Updated

11 years ago
Component: General → JavaScript Engine
(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

11 years ago
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.
(Assignee)

Updated

11 years ago
Assignee: nobody → general
QA Contact: general → general
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Blocks: 336409
(Assignee)

Comment 5

11 years ago
Created attachment 227486 [details] [diff] [review]
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).
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #227486 - Flags: review?(brendan)
(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.

Flags: blocking1.8.0.5? → blocking1.8.0.5+
Comment on attachment 227486 [details] [diff] [review]
Fix

approved for 1.8.0 branch pending brendan's review, a=dveditz for drivers
Attachment #227486 - Flags: approval1.8.0.5+
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
Attachment #227486 - Flags: review?(brendan) → review+
-            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?
(Assignee)

Comment 10

11 years ago
> is the second backslash misaligned on purpose?

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

Comment 11

11 years ago
Fix checked into the 1.8.0 branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
Whiteboard: [sg:critical?]

Comment 12

11 years ago
Comment on attachment 227486 [details] [diff] [review]
Fix

Approving for branch since this is on trunk and 1.8.0 branch.
Attachment #227486 - Flags: approval1.8.1+

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+

Comment 13

11 years ago
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

11 years ago
ditto js1_5/Regress/regress-336410-[12].js
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

11 years ago
Created attachment 228295 [details]
js1_5/GC/regress-342960.js

Updated

11 years ago
Flags: in-testsuite+

Updated

11 years ago
Keywords: fixed1.8.1

Comment 17

11 years ago
verified fixed 1.8.0.5, 1.9a1 windows/macppc/linux.

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



Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.5, fixed1.8.1 → verified1.8.0.5

Comment 18

11 years ago
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_.
(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

Updated

11 years ago
Blocks: 344320

Comment 20

11 years ago
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()?
> 
> 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.
(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

11 years ago
mrbkap: can you please land this on 1.8 today? 
(Assignee)

Comment 24

11 years ago
Sorry, this was checked in with js1.7.
Keywords: fixed1.8.1

Comment 25

11 years ago
verified fixed 1.8 win/mac(ppc|tel)/linux 20060725
Keywords: fixed1.8.1 → verified1.8.1

Comment 26

11 years ago
Created attachment 232732 [details] [diff] [review]
1.0.x version of the patch

Comment 27

11 years ago
Created attachment 245293 [details]
js1_5/GC/regress-342960.js

modify expected exit code for the shell per bug 358975.
Attachment #228295 - Attachment is obsolete: true
Group: security

Comment 28

10 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-342960.js,v  <--  regress-342960.js
You need to log in before you can comment on or make changes to this bug.