131246 - Crash after out-of-memory in JS_NewContext()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steven Cole]

So I implemented a random heap failure hack for the JS shell / JS library (ala
my heap limiter from yesterday); this is my first crash from that.

If JS runs out of ram during js_initPinnedAtoms, then the engine will later try
to destroy the context, but fail because things are left in an awkward state.

I will attach the patch to make the JS library / shell fail randomly; this
particular crash is for a failure frequency of 0.1 %, and a seed of 4567. 
(Hopefully, the Win32 rand function is implemented the same for everyone and
this data makes the same crash for you!)

The location of the memory failure is:
JS_malloc(JSContext * 0x00135dc0, unsigned int 0x0000000c) line 1421 + 9 bytes
js_NewStringCopyN(JSContext * 0x00135dc0, const unsigned short * 0x0012fd0c,
unsigned int 0x00000005, unsigned int 0x00000000) line 2452 + 17 bytes
js_AtomizeString(JSContext * 0x00135dc0, JSString * 0x0012fd50, unsigned int
0x00000081) line 576 + 66 bytes
js_Atomize(JSContext * 0x00135dc0, const char * 0x100f0d80 _js_arity_str,
unsigned int 0x00000005, unsigned int 0x00000001) line 645 + 19 bytes
js_InitPinnedAtoms(JSContext * 0x00135dc0, JSAtomState * 0x00132e4c) line 273 +
30 bytes
js_InitAtomState(JSContext * 0x00135dc0, JSAtomState * 0x00132e4c) line 232 + 13
bytes
js_NewContext(JSRuntime * 0x00132de0, unsigned int 0x00002000) line 130 + 25 bytes
JS_NewContext(JSRuntime * 0x00132de0, unsigned int 0x00002000) line 886 + 13 bytes
main(int 0x00000000, char * * 0x00300054) line 2063 + 17 bytes


And the location of the crash is

JS_HashTableEnumerateEntries(JSHashTable * 0x00000000, int (JSHashEntry *, int,
void *)* 0x10013ba0 js_atom_unpinner(JSHashEntry *, int, void *), void *
0x00000000) line 361 + 8 bytes
js_UnpinPinnedAtoms(JSAtomState * 0x00132e4c) line 430 + 19 bytes
js_DestroyContext(JSContext * 0x00135dc0, int 0x00000000) line 171 + 12 bytes
js_NewContext(JSRuntime * 0x00132de0, unsigned int 0x00002000) line 138 + 11 bytes
JS_NewContext(JSRuntime * 0x00132de0, unsigned int 0x00002000) line 886 + 13 bytes
main(int 0x00000000, char * * 0x00300054) line 2063 + 17 bytes

Comment 1

[Steven Cole]

Attachment: Patch to make JS & shell have a random memory failure.

The frequency and seed are embedded in the source, so if you want to try
different values, you need to do a quick recompile.  (Yes, setting these on the
command line would have been nicer.)

Comment 2

[Phil Schwartau]

Reassigning to Kenton - 

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Thanks -- any more? :-)

I'm inclined to check this in forthwith -- r= and sr= welcome to legitimize me.


/be

Comment 4

[Steven Cole]

Sorry, Brendan, no more for the rest of the weekend :)  I ran this randomizer
for quite a while and this was the only thing that popped up.

But then it occurred to me that with the generally excellent "bail out -- no
memory" logic that's in the engine, a random failure tester is testing only the
start of tests --- not the end of them.

So I'll try something more systematic next week.

--steve

Comment 5

[John Bandhauer]

Comment on attachment 74495 [details] [diff] [review]
proposed fix

rs=jband

Comment 6

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 7

[Steven Cole]

Sorry, Brendan, that fix just moves the crash.  New crash happens during garbage
collection:

JS_HashTableEnumerateEntries(JSHashTable * 0x00000000, int (JSHashEntry *, int,
void *)* 0x100139d0 js_atom_marker(JSHashEntry *, int, void *), void *
0x0012fc08) line 361 + 8 bytes
js_MarkAtomState(JSAtomState * 0x00132ea4, unsigned int 0x00000000, void (void
*, void *)* 0x1003e090 gc_mark_atom_key_thing(void *, void *), void *
0x00135e18) line 388 + 21 bytes
js_GC(JSContext * 0x00135e18, unsigned int 0x00000000) line 1156 + 25 bytes
js_ForceGC(JSContext * 0x00135e18) line 977 + 11 bytes
js_DestroyContext(JSContext * 0x00135e18, int 0x00000000) line 210 + 9 bytes
js_NewContext(JSRuntime * 0x00132e38, unsigned int 0x00002000) line 138 + 11 bytes
JS_NewContext(JSRuntime * 0x00132e38, unsigned int 0x00002000) line 886 + 13 bytes


--scole

Comment 8

[Brendan Eich [:brendan]]

scole: No worries, we can keep this bug open until you say close -- or feel free
to file new bugs for new problems.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: I hope this does the trick

This unifies jsatom.c code to use test state->table nullity as needed to cope
with js_DestroyContext called from failing js_NewContext.  Scole, can you test
this quickly?

/be

Comment 10

[Brendan Eich [:brendan]]

One subtle thing: I moved state->runtime = rt; down from the top of
js_InitAtomState till after state->table has been successfully allocated.  This
ensures the invariant that rt->atomState is either all-zeroes if uninitialized
or failed-to-be-initialized, and state->runtime and state->table are non-null if
rt->atomState is initialized.  Still, uses of state->table that might come from
a js_DestroyContext called from a failing js_NewContext test state->table
directly, not state->runtime -- it's best to test the depended-on pointer, I say.

Kenton, can you r= today?

/be

Comment 11

[Steven Cole]

Brendan,

Still crashing; this time: the call to JS_HashTableEnumerateEntries from
js_SweepAtomState.  Stack:

JS_HashTableEnumerateEntries(JSHashTable * 0x00000000, int (JSHashEntry *, int,
void *)* 0x10013af0 js_atom_sweeper(JSHashEntry *, int, void *), void *
0x00132ea4) line 361 + 8 bytes
js_SweepAtomState(JSAtomState * 0x00132ea4) line 418 + 21 bytes
js_GC(JSContext * 0x00135e18, unsigned int 0x00000000) line 1251 + 12 bytes
js_ForceGC(JSContext * 0x00135e18) line 977 + 11 bytes
js_DestroyContext(JSContext * 0x00135e18, int 0x00000000) line 210 + 9 bytes
js_NewContext(JSRuntime * 0x00132e38, unsigned int 0x00002000) line 138 + 11 bytes
JS_NewContext(JSRuntime * 0x00132e38, unsigned int 0x00002000) line 886 + 13 bytes
orig_main(int 0x00000004, char * * 0x00301c44) line 2063 + 17 bytes
main(int 0x00000005, char * * 0x00301c40) line 2179 + 13 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 12

[Steven Cole]

But adding another null check to js_SweepAtomState makes the problem go away.

--scole

Comment 13

[John Bandhauer]

Comment on attachment 74833 [details] [diff] [review]
I hope this does the trick

r/sr=jband. Sounds like there is one more null check coming. Please xfer my sr=
if that is all you add for the next patch.

Comment 14

[Brendan Eich [:brendan]]

Attachment: final patch

How'd I miss that?  I had in mind to test state->table in js_SweepAtomState
too, oh well.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 74890 [details] [diff] [review]
final patch

Forwarding jband's sr= stamp.  I'll take an r= from scole if he's happy.

/be

Comment 16

[Steven Cole]

Comment on attachment 74890 [details] [diff] [review]
final patch

r=[e-mail]

(Review based on emprical successful behavior, rather than a keen understanding
of the JS Engine internals, though.)

Comment 17

[Brendan Eich [:brendan]]

I'm still holding the patch here, and it has sr= and r= (although not
r=khanson).    I suppose I should take this bug and get drivers a=....

/be

Comment 18

[Scott Collins]

Comment on attachment 74890 [details] [diff] [review]
final patch

a=scc

Comment 19

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 20

[Phil Schwartau]

Marking Verified per Steven's Comment #16 -