329530 - Out of memory crash when calling fn.toString where fn is a deeply nested function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Erik Fabert]

The testcase is really simple: it build a function nested 1000 levels deep of the form
   function test() { function test() { function test() { ... }}}
and then tries to call that function's toString method. After some time all available memory will be consumed (several hundred MB in my case), the JS shell prints 'out of memory' once or twice and then crashes with a read access violation.

For comparison, IE6 and Opera 8.5 have no problem and display the result without any noticable delay. No wonder in IE's case because it stores the source of the function; Opera builds a nicely indented string representation and needs just roughly 16 MB for this task.

Comment 1

[Erik Fabert]

Attachment: testcase

I forgot to mention, I tested this with an up-to-date, optimized JS shell.

Comment 2

[Erik Fabert]

Attachment: possible patch

I really don't know if this is the right thing to do, but with this patch the shell just prints "out of memory" once and then peacefully returns to its "js>" prompt.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 214319 [details] [diff] [review]
possible patch

Clearly a good fix -- failure must propagate. r=me.

Blake, anyone: why is so much memory being consumed?  That seems like the harder bug to fix here.

/be

Comment 4

[Erik Fabert]

Attachment: found another problem

js_DestroyPrinter hates null pointers. This patch is still not sufficient, even with it the testcase ocasionally crashes in memmove.

Comment 5

[Erik Fabert]

Attachment: another one, save PC

I've had a crash in js_PCToLineNumber because pc was 0xdadadada after the realloc in js_ConcatStrings failed. I think this patch ought to fix that.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 214365 [details] [diff] [review]
found another problem

Thanks, obvious old bug.  I'm surprised it wasn't spotted sooner.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 214319 [details] [diff] [review]
possible patch

Obsoleted by (subsumed by) the patch after this one.

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 214432 [details] [diff] [review]
another one, save PC

Is this the last patch?  I'm cool with an all-in-one patch too, if there are more fixes coming.  Thanks again,

/be

Comment 9

[Erik Fabert]

No, that was the last one. I've seen an assertion failure in FreeArenaList once but couldn't reproduce it since then. I couldn't debug that problem anyway, I don't understand the arena code at all. Too much pointer arithmetic voodoo for my poor little brain...

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (From update of attachment 214432 [details] [diff] [review] [edit])
> Is this the last patch?  I'm cool with an all-in-one patch too, if there are
> more fixes coming.  Thanks again,

On second thought, it's better to keep the jsinterp.c patch separate, because it is not needed on the 1.8* branches.

mrbkap, can you take this one?  Wireless connectivity at ETech is bad and I'm not likely to be able to complete any checkins.

Bob, I'm hoping we can pick up the jsopcode.c patch for js1.6 -- if not for rc1, no sweat, but I wanted to set blocking to keep track of this bug.

/be

Comment 11

[timeless]

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

Comment 12

[Erik Fabert]

Just a gentle reminder that these patches still need to be checked in.

Comment 13

[timeless]

Comment on attachment 214365 [details] [diff] [review]
found another problem

mozilla/js/src/jsopcode.c 	3.113

Comment 14

[timeless]

Comment on attachment 214432 [details] [diff] [review]
another one, save PC

mozilla/js/src/jsinterp.c 	3.229

Comment 15

[timeless]

thanks for the patches, please resolve this bug as fixed if current cvs indeed no longer crashes for this scenario.

Comment 16

[Erik Fabert]

Thanks for the checkin. This is not fixed yet, the patches only reduce the crash probability from 100% to under 20%. I still suspect the area code doesn't handle failed realloc attempts but I can't prove it.

The question whether something can be done about the enormous memory usage is still open, too. Both issues are over my head, I'll hand this bug back to [e-mail].

Comment 17

[Bob Clary [:bc] (inactive)]

Checking in regress-329530.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-329530.js,v  <--  regress-329530.js
initial revision: 1.1
done

Marking fixed as trunk no longer crashes.

Comment 18

[Bob Clary [:bc] (inactive)]

Comment on attachment 214432 [details] [diff] [review]
another one, save PC

Requesting that the full fix be applied to the 1.8.0 and 1.8 branches as well.

Comment 19

[Bob Clary [:bc] (inactive)]

(In reply to comment #14)
> (From update of attachment 214432 [details] [diff] [review] [edit])
> mozilla/js/src/jsinterp.c       3.229
> 

Note this contains a call to SAVE_SP_AND_PC which is not defined on 1.8

Comment 20

[Daniel Veditz [:dveditz]]

Please provide a non-obsolete patch for 1.8.0 branch approval.

Comment 21

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-329530.js still crashes on windows trunk browser and shell in JS_ArenaRealloc  [mozilla/js/src/jsarena.c, line 246] , but not in linux or mac. See TB17031159. I can reproduce this in opt builds but not in debug builds.

Should I reopen this or file a new bug?

Comment 22

[Daniel Veditz [:dveditz]]

I'm confused, which patch do we want? why are both on trunk and only one on 1.8 branch?

Comment 23

[Erik Fabert]

(In reply to comment #22)
> I'm confused, which patch do we want? why are both on trunk and only one on 1.8
> branch?

See bug 309169 comment 13: the jsinterp.c patch (attachment 214432 [details] [diff] [review]) is not needed on the branches because pc is always saved for all opcodes there. 

Attachment 214365 [details] [diff] OTOH fixes two old bugs is jsopcode.c, that's the one to consider for 1.8.03.

Comment 24

[Bob Clary [:bc] (inactive)]

add block to 1.8.1 for tracking related to js16 

Comment 25

[Bob Clary [:bc] (inactive)]

Comment on attachment 214432 [details] [diff] [review]
another one, save PC

this patch isn't appropriate for 1.8.x, sorry.

Comment 26

[Bob Clary [:bc] (inactive)]

(In reply to comment #17)
> 
> Marking fixed as trunk no longer crashes.

Still crashing trunk / windows in shell.... Unfortunately I can't get stacks with the shell at the moment.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 214365 [details] [diff] [review]
found another problem

approved for 1.8.0 branch, a=dveditz for drivers

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 214365 [details] [diff] [review]
found another problem

unobsoleting since this still needs to be checked in.

Comment 29

[Brendan Eich [:brendan]]

Fixes to jsinterp.c and jsopcode.c landed on the 1.8.0 branch:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.7; previous revision: 3.181.2.17.2.6
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.8.2.4; previous revision: 3.89.2.8.2.3
done

/be

Comment 30

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #29)
> Fixes to jsinterp.c and jsopcode.c landed on the 1.8.0 branch:

The change to jsinterp.c turned all 1.8.0 tinderboxes red, since SAVE_SP_AND_PC is not defined on that branch. Should it just be backed out, per comment bug 309169 comment 13?

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> (In reply to comment #29)
> > Fixes to jsinterp.c and jsopcode.c landed on the 1.8.0 branch:
> 
> The change to jsinterp.c turned all 1.8.0 tinderboxes red, since SAVE_SP_AND_PC
> is not defined on that branch. Should it just be backed out, per comment bug
> 309169 comment 13?

No, that comment does not say to back anything out -- it just notes that only sp needs to be saved on the 1.8.0 branch.  Anyway, looks like dbaron wiped up after me here (see bug 331719 comment 10).  Thanks, dbaron.

/be

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #31)
> No, that comment does not say to back anything out -- it just notes that only
> sp needs to be saved on the 1.8.0 branch.

Perhaps I should have been clearer. Two SAVE_SP_AND_PCs additions were checked in to jsinterp.c on the 1.8.0 branch: the one from attachment 214432 [details] [diff] [review] of this bug, and the one from bug 331719. dbaron had originally only fixed the one from bug 331719, which led to my comment 30 here. He later backed out attachment 214432 [details] [diff] [review] of this bug, which is the solution that I was proposing.

Comment 33

[Brendan Eich [:brendan]]

Gavin: you're right, I was talking about only the necessary SAVE_SP(fp) -- my fault for not being clearer, not yours.

This bug should be owned.  I'll take it for now.

/be

Comment 34

[Bob Clary [:bc] (inactive)]

20060425 builds win/mac/linux browser and shell tested. results:

out of memory, no crash - shell windows 1.8.0.4, 1.8.1, 1.9a1
crash - browser windows 1.8.1
all others - no crash, no error.

verified fixed 1.8.0.4

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does this need to be fixed on the 1.8 branch?

Comment 36

[Bob Clary [:bc] (inactive)]

(In reply to comment #35)
> Does this need to be fixed on the 1.8 branch?
> 

yes.

Comment 37

[Brendan Eich [:brendan]]

I'm on vacation, overloaded with other stuff and a poor owner for this bug.  If mrbkap or igor can help, I'll once again owe someone big-time.

/be

Comment 38

[Igor Bukanov]

I committed the patch from comment 4 to MOZILLA_1_8_BRANCH

Comment 39

[Bob Clary [:bc] (inactive)]

Note I saw a crash with a 20060630 shell opt trunk build on windows but can't reproduce it with 20060701 build.  Other than that, I don't see any other crashes on the shell or browser for 20060630 builds on win/mac(ppc|tel)/linux. 

verified fixed 1.8.1

Is this bug fixed?

Comment 40

[Bob Clary [:bc] (inactive)]

again windows opt trunk shell 20060728 crashes on the qa farm (windows 2003 server) but I can't reproduce locally with windows xp.

The dialog message on windows server 2003 is
"js.exe - Illegal System DLL Relocation. The system DLL user32.dll was relocated in memory. The application will not run properly. The relocation occurred because the DLL Dynamically Allocated Memory occupied an address range reserved for Windows system DLLs. The vendor supplying the DLL should be contacted for a new DLL."

Comment 41

[Bob Clary [:bc] (inactive)]

In addition to the continuing trunk crashes, beginning 2006-08-11, I began seeing crashes in windows 1.8 shell opt 

 	js32.dll!_js_ReportOutOfMemory()  + 0x6b bytes	
 	js32.dll!_js_printf()  + 0x38 bytes	
>	js32.dll!_js_DecompileFunction()  + 0x246 bytes	
 	js32.dll!_js_DecompileCode()  + 0x3174 bytes	
 	js32.dll!_js_DecompileCode()  + 0xae bytes	
 	js32.dll!_js_DecompileScript()  + 0x16 bytes	
 	js32.dll!_js_DecompileFunction()  + 0x20d bytes	
repeated.

Comment 42

[Igor Bukanov]

There is yet another problem in jsopcode.c. In may places the code there ignores a failure return of js_printf/js_putc. Theoretically this should be harmless, but allocating various items under Out-Of-Memory condition may not be safe.

Comment 43

[Igor Bukanov]

(In reply to comment #41)
> In addition to the continuing trunk crashes, beginning 2006-08-11, I began
> seeing crashes in windows 1.8 shell opt 

What is is the maximum memory that js1_5/Regress/regress-329530.js uses when crash happens?

And out of curiosity, what would print the following C program when executed on the test box:

#include <stdio.h>
#include <stdlib.h>

int main()
{
    unsigned i, sz;
    void *p, *p2;

    p = NULL;
    for (i = 0; i != 32; ++i) {
        sz = (1 << i) + 1;
        p2 = (!p) ? malloc(sz) : realloc(p, sz);
        if (!p2) {
            printf("FAILED to allocate when size=%X p=%p\n", sz, p);
            break;
        }
        if (p && p != p2)
            printf("Moved when size=%X p=%p p2=%p\n", sz, p, p2);
        p = p2;
    }
    return 0;
}

Comment 44

[Bob Clary [:bc] (inactive)]

(In reply to comment #43)
> (In reply to comment #41)
> > In addition to the continuing trunk crashes, beginning 2006-08-11, I began
> > seeing crashes in windows 1.8 shell opt 
> 
> What is is the maximum memory that js1_5/Regress/regress-329530.js uses when
> crash happens?
> 

for winxp builds from this morning:
1.9 opt shell: about 1.9G and then out of memory with no crash.
1.9 dbg shell: error too much recursion
1.8 opt shell: about 1.9G and then crash in js_ReportOutOfMemory()
1.8 dbg shell: error too much recursion


> And out of curiosity, what would print the following C program when executed on
> the test box:

Moved when size=401 p=00358D58 p2=00355028
Moved when size=801 p=00355028 p2=00358D58
Moved when size=2001 p=00358D58 p2=00359D88
Moved when size=4001 p=00359D88 p2=00420068
Moved when size=8001 p=00420068 p2=00424098
Moved when size=10001 p=00424098 p2=0042C0C8
Moved when size=20001 p=0042C0C8 p2=0043C0F8
Moved when size=40001 p=0043C0F8 p2=0045C128
Moved when size=80001 p=0045C128 p2=00360040
Moved when size=100001 p=00360040 p2=00520040
Moved when size=200001 p=00520040 p2=00630040
Moved when size=400001 p=00630040 p2=00840040
Moved when size=800001 p=00840040 p2=00C50040
Moved when size=1000001 p=00C50040 p2=01460040
Moved when size=2000001 p=01460040 p2=02470040
Moved when size=4000001 p=02470040 p2=04480040
Moved when size=8000001 p=04480040 p2=10320040
Moved when size=10000001 p=10320040 p2=18330040
Moved when size=20000001 p=18330040 p2=28340040
FAILED to allocate when size=40000001 p=28340040

Comment 45

[Igor Bukanov]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #41)
> > > In addition to the continuing trunk crashes, beginning 2006-08-11, I began
> > > seeing crashes in windows 1.8 shell opt 
> > 
> > What is is the maximum memory that js1_5/Regress/regress-329530.js uses when
> > crash happens?
> > 
> 
> for winxp builds from this morning:
> 1.9 opt shell: about 1.9G and then out of memory with no crash.
> 1.9 dbg shell: error too much recursion
        ^^^^^^^^^^^^^
Would you see the crash when you pass  -S 2000000 to the shell for the debug build? The default stack limit of 512K is not enough to run the tests on Linux as well.

> > And out of curiosity, what would print the following C program when executed on
> > the test box:
> 
> Moved when size=401 p=00358D58 p2=00355028
> Moved when size=801 p=00355028 p2=00358D58
> Moved when size=2001 p=00358D58 p2=00359D88
> Moved when size=4001 p=00359D88 p2=00420068
> Moved when size=8001 p=00420068 p2=00424098
> Moved when size=10001 p=00424098 p2=0042C0C8
> Moved when size=20001 p=0042C0C8 p2=0043C0F8
> Moved when size=40001 p=0043C0F8 p2=0045C128
> Moved when size=80001 p=0045C128 p2=00360040
> Moved when size=100001 p=00360040 p2=00520040
> Moved when size=200001 p=00520040 p2=00630040
> Moved when size=400001 p=00630040 p2=00840040
> Moved when size=800001 p=00840040 p2=00C50040
> Moved when size=1000001 p=00C50040 p2=01460040
> Moved when size=2000001 p=01460040 p2=02470040
> Moved when size=4000001 p=02470040 p2=04480040
> Moved when size=8000001 p=04480040 p2=10320040
> Moved when size=10000001 p=10320040 p2=18330040
> Moved when size=20000001 p=18330040 p2=28340040
> FAILED to allocate when size=40000001 p=28340040

Interesting, so that test box can not allocate > 1GB of continues memory, exactly as happens on my Ubuntu laptop, Why then on Linux the test does not crash?.. 

Comment 46

[Bob Clary [:bc] (inactive)]

(In reply to comment #45)

> > 1.9 dbg shell: error too much recursion
>         ^^^^^^^^^^^^^
> Would you see the crash when you pass  -S 2000000 to the shell for the debug
> build? The default stack limit of 512K is not enough to run the tests on Linux
> as well.

Same error

> 
> 
> Interesting, so that test box can not allocate > 1GB of continues memory,
> exactly as happens on my Ubuntu laptop, Why then on Linux the test does not
> crash?.. 
> 

from malloc.h

/* Maximum heap request the heap manager will attempt */

#ifdef  _WIN64
#define _HEAP_MAXREQ    0xFFFFFFFFFFFFFFE0
#else
#define _HEAP_MAXREQ    0xFFFFFFE0
#endif

That says 4G on my machine if I read it correctly. But my machine has only 3.25 available due to hardware eating addresses. I reran it after making sure that most running programs were closed. The program needed .5G + 1.0G to do the realloc and according to task manager only used about .75G. The difference might be related to the memory used by the system cache which used about .75G plus the kernel memory.

Comment 47

[Bob Clary [:bc] (inactive)]

(In reply to comment #46)

In windows 2000/xp 32bit the restriction is 4G per process with 2G for user and 2G for the kernel. It might be possible to change this to 3G/1G. More news later.

Comment 48

[Bob Clary [:bc] (inactive)]

<http://support.microsoft.com/default.aspx?scid=kb;en-us;833721>
<http://msdn.microsoft.com/library/default.asp?url=/library/en-us/DevTest_g/hh/DevTest_g/BootIni_de16d3ec-c437-4628-805f-8945ea598a92.xml.asp>

You need to compile a program with /LARGEADDRESSAWARE to address more than 2G if you use the /3G boot option.

Comment 49

[Igor Bukanov]

(In reply to comment #46)
> (In reply to comment #45)
> 
> > > 1.9 dbg shell: error too much recursion
> >         ^^^^^^^^^^^^^
> > Would you see the crash when you pass  -S 2000000 to the shell for the debug
> > build? The default stack limit of 512K is not enough to run the tests on Linux
> > as well.
> 
> Same error

You mean no crash with a stack trace but rather too-much-recursion error? What about passing -S 0 to disable stack checks?

Comment 50

[Igor Bukanov]

I think the test case crashes come from just filed bug 348986.

Comment 51

[timeless]

technically you don't need to recompile, you can simply run a tool to change the binary later. but yes you definitely need to have booted w/ /3G (which i'd recommend if you plan to use purify/quantify).

Comment 52

[Bob Clary [:bc] (inactive)]

(In reply to comment #49)
 
> You mean no crash with a stack trace but rather too-much-recursion error? 

yes,

> What about passing -S 0 to disable stack checks?

no crash, out of memory.

Comment 53

[Bob Clary [:bc] (inactive)]

1.8 opt shell windows crash 20060821 (you need oodles of ram)
.../js -f js1_5/shell.js js1_5/Regress/regress-329530.js

 	msvcrt.dll!_memmove()
>	js32.dll!_js_QuoteString()
 	js32.dll!_js_QuoteString()
 	js32.dll!_js_QuoteString()
 	js32.dll!_js_printf()
 	js32.dll!_js_DecompileFunction() -
 	js32.dll!_js_DecompileCode()      |
 	js32.dll!_js_DecompileCode()      repeat
 	js32.dll!_js_DecompileScript()   -

1.8, 1.9 debug shell windows crash 20060821 
.../js -S 1000000 -f js1_5/shell.js js1_5/Regress/regress-329530.js

 	ntdll.dll!_RtlpFindEntry@8()  + 0x2b9 bytes	
 	ntdll.dll!_RtlpInsertFreeBlock@12()  + 0x88 bytes	
 	ntdll.dll!_RtlpDeCommitFreeBlock@12()  + 0xfc bytes	
 	ntdll.dll!_RtlFreeHeap@12()  + 0xa8d bytes	
 	msvcrt.dll!_free()  + 0xc3 bytes	
>	js32.dll!js_printf(JSPrinter * jp=0x00551c50, const char * format=0x610da268, ...)  Line 652 + 0xa bytes	
 	[Frames below may be incorrect and/or missing, no symbols loaded for js32.dll]	
 	js32.dll!Decompile(SprintStack * ss=0x000ca474, unsigned char * pc=0x0049a20d, int nb=7)  Line 1232 + 0x1e bytes	
 	js32.dll!js_DecompileCode(JSPrinter * jp=0x00551c50, JSScript * script=0x0049a1d8, unsigned char * pc=0x0049a208, unsigned int len=7)  Line 3106 + 0x11 bytes	
 	js32.dll!js_DecompileScript(JSPrinter * jp=0x00551c50, JSScript * script=0x0049a1d8)  Line 3126 + 0x1a bytes	
 	js32.dll!js_DecompileFunction(JSPrinter * jp=0x00551c50, JSFunction * fun=0x0044b310)  Line 3231 + 0x10 bytes	
 	js32.dll!Decompile(SprintStack * ss=0x000ca7d8, unsigned char * pc=0x0049a265, int nb=7)  Line 1227 + 0xd bytes	

new bug for 1.8 or remove the verified1.8.1?

Comment 54

[Bob Clary [:bc] (inactive)]

I am no longer crashing with this in the browser or shell on windows trunk. Marking fixed.

Comment 55

[Bob Clary [:bc] (inactive)]

verified fixed 20061122 1.9 windows/linux