Using the OOM testcode in bug 383932, a few problems in OOM handling have been found which can lead to memory corruption or memory leaks. These are: 1) In js_AtomizeString, failure of JS_HashTableRawAdd causes the function to return NULL, but the newly created JS string holds a pointer to the 'chars' passed in by the caller (if ATOM_NOCOPY is specified). This leads to a double free of the same memory pointer (once in the caller of js_AtomizeString and later when the JSString is finalised). Testing with e4x/Regress/regress-277650.js showed this bug. 2) In Decompile, when populating the 'argv' array (around line 3330-ish), we can fail half-way through leaving some entries in the array with unitialised pointers. This leads to a crash later when they are passed through the sprinter. Testing with js1_6/decompilation/regress-352084.js showed this bug. 3) In NewOrRecycleNode failure to allocate the parse node results in a crash due to memsetting a NULL pointer. Testing with e4x/XML/13.4.1.js showed this bug (actually just about any script should do). 4) In js_InitRuntimeStringState, failure of js_AtomizeString leaves the JSString locked so it is never finalised (memory leak). Testing with e4x/XML/13.4.4.1.js showed this bug, although any script should do. 5) In ParseXMLSource, 'chars' is leaked if js_NewBufferTokenStream fails. Found when testing e4x/XML/13.4.4.3.js 6) In AppendAttributeValue, failure of js_AppendCString (or any previous string buffer call) and js_EscapeAttributeValue leads to an attempt to free a pointer with address '1'. Noticed when testing e4x/XML/13.4.2.js. 7) In XMLToXMLString, failure of js_RepeatChar leaves the string buffer invalid, so there are a few points where this can lead to crashes - one example is when attempting to grow the string buffer. Noticed when testing e4x/XML/regress-336921.js. 8) In XMLToXMLString, a call to js_FinishStringBuffer can potentially be passed an invalid string buffer, which leads to a call to free with an address of '1'. Reproducible when testing with e4x/XML/13.4.4.12.js. 9) In xml_toString_helper, there is no check of the return code from js_EnterLocalRootScope, which leaves GCThings unprotected if OOM should happen. Noticed when testing with XML/regress-376773.js (assert is fired when js_LeaveLocalRootScope is called). 10) Minor issue: in XMLToXMLString, for 'case JSXML_CLASS_LIST', the error checking for STRING_BUFFER_OK should probably be outside the check for base being NULL (since a NULL base is always 'ok').

Fixes for the issues numbered above are: 1) Make sure str->chars is cleared to NULL before returning 2) Complete the loop to ensure all argv values are set up. Continuing in the face of OOM may seem odd, but there is much that happens after this point anyway, so it is the safest fix I think 3) Check for pn being NULL 4) Unlock the GCThing before returning 5) Goto common exit block to release arena and free chars 6) Check for the string buffer being valid before freeing it 7) Check for failure after calling js_RepeatChar. This approach was chosen in preference to checking in each of the individual lower level functions called from here (less code this way, but maybe less immune to future changes) 8) Check for the string buffer being valid before calling finish 9) Check return code and return early 10) Re-arrange error checking to testing for the string buffer being invalid first.

Gavin, standard operating procedure is to request review using the https://bugzilla.mozilla.org/attachment.cgi?id=268708&action=edit page's drop-down and the email-address text field that appears to its right. I'll do that this time and review myself. Thanks a ton for this work. /be

Comment on attachment 268708 [details] [diff] [review] Proposed fixes >@@ -3326,17 +3326,16 @@ Decompile(SprintStack *ss, jsbytecode *p > if (!argv) > return NULL; > > ok = JS_TRUE; > for (i = argc; i > 0; i--) { > argv[i] = JS_strdup(cx, POP_STR()); > if (!argv[i]) { > ok = JS_FALSE; >- break; > } Nit: remove braces for single-line if with single-line else (house style). >@@ -1995,45 +1995,46 @@ ParseXMLSource(JSContext *cx, JSString * > srcp = JSSTRING_CHARS(src); > js_strncpy(chars + offset, srcp, srclen); > offset += srclen; > dstlen = length - offset + 1; > js_InflateStringToBuffer(cx, suffix, constrlen(suffix), chars + offset, > &dstlen); > chars [offset + dstlen] = 0; > >+ xml = NULL; > mark = JS_ARENA_MARK(&cx->tempPool); Super-nit: move JSXML *xml; decl up to just before mark's decl to match first-assignment order. >- if (!sb.base) { >- if (!STRING_BUFFER_OK(&sb)) { >- JS_ReportOutOfMemory(cx); >- return NULL; >- } >+ if (!sb.base) > return cx->runtime->emptyString; >+ >+ if (!STRING_BUFFER_OK(&sb)) { >+ JS_ReportOutOfMemory(cx); >+ return NULL; > } (Clearly left over from a prior state where !STRING_BUFFER_OK was not encoded as a nearly-null base pointer!) r=me with nits picked. /be

Patch with nits picked.

Comment on attachment 268779 [details] [diff] [review] Fix v2 r=me again -- crowder, could you please help get this checked in. Where's that shaver to sr+ gavin's CVS commit access request? /be

Checking in jsatom.c; /cvsroot/mozilla/js/src/jsatom.c,v <-- jsatom.c new revision: 3.97; previous revision: 3.96 done Checking in jsopcode.c; /cvsroot/mozilla/js/src/jsopcode.c,v <-- jsopcode.c new revision: 3.252; previous revision: 3.251 done Checking in jsparse.c; /cvsroot/mozilla/js/src/jsparse.c,v <-- jsparse.c new revision: 3.289; previous revision: 3.288 done Checking in jsstr.c; /cvsroot/mozilla/js/src/jsstr.c,v <-- jsstr.c new revision: 3.147; previous revision: 3.146 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.158; previous revision: 3.157 done