Closed Bug 342960 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: guninski, Assigned: mrbkap)

References

Details

(Keywords: verified1.8.0.5, verified1.8.1, Whiteboard: [sg:critical?])

Attachments

(4 files, 1 obsolete file)

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
Doesn't crash recent trunk on winxp. Georgi: any reason you didn't file this under JavaScript Engine?
Product: Firefox → Core
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?
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: 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
Attached patch FixSplinter Review
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?
> is the second backslash misaligned on purpose?

Oops, no it isn't. I'll fix it before checking in.
Fix checked into the 1.8.0 branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Keywords: fixed1.8.0.5
Whiteboard: [sg:critical?]
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+
Flags: blocking1.8.1? → blocking1.8.1+
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
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
Attached file js1_5/GC/regress-342960.js (obsolete) —
Flags: in-testsuite+
Keywords: fixed1.8.1
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
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
Blocks: 344320
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.
mrbkap: can you please land this on 1.8 today? 
Sorry, this was checked in with js1.7.
Keywords: fixed1.8.1
verified fixed 1.8 win/mac(ppc|tel)/linux 20060725
modify expected exit code for the shell per bug 358975.
Attachment #228295 - Attachment is obsolete: true
Group: security
/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.