614928 - PropertyTree::insertChild returns without unlocking cx->runtime when hash->add fails

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[timeless]

http://mxr.mozilla.org/mozilla-central/source/js/src/jspropertytree.cpp?mark=422,432,146,175-177,433,436#421

http://mxr.mozilla.org/mozilla-central/source/js/src/jsparse.cpp?mark=555-559,479#555

340  	PropertyTree::getChild(JSContext *cx, Shape *parent, const Shape &child)
341  	{

421  	  not_found:
422  	    JS_LOCK_GC(cx->runtime);

lock acquired
423  	
424  	  locked_not_found:
425  	    shape = newShape(cx, true);
newShape unlocks on failure, so this is ok:
426  	    if (!shape)
427  	        return NULL;

429  	    new (shape) Shape(child.id, child.rawGetter, child.rawSetter, child.slot, child.attrs,
430  	                      child.flags, child.shortid, js_GenerateShape(cx, true));
431  	

insertChild doesn't seem to unlock on failure, so this is bad:
432  	    if (!insertChild(cx, parent, shape))
433  	        return NULL;
434  	
435  	  out:
here's the unlock call we skipped:
436  	    JS_UNLOCK_GC(cx->runtime);

Comment 1

[timeless]

oh, no newShape won't unlock either since it was passed true and thus wouldn't be locking or unlocking. Sorry, i've been up too long.

Comment 2

[timeless]

Attachment: patch

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 493530 [details] [diff] [review]
patch

To me, it looks like PropertyTree::newShape does unconditionally call JS_UNLOCK_GC on error.

And PropertyTree::insertChild does unlock on failure when OOM occurs under KidsChunk::create, but not when it occurs under hash->add(). In that case, no error is reported at all. Gross.

So I think the fix here is to leave PropertyTree::getChild alone and fix the !hash->add() path in PropertyTree::insertChild.

While you're in here, please change this comment:

> /*
>  * NB: Called with cx->runtime->gcLock held, always.
>- * On failure, return null after unlocking the GC and reporting out of memory.
>+ * On failure, return false after unlocking the GC and reporting out of memory.
>  */
> bool
> PropertyTree::insertChild(JSContext *cx, Shape *parent, Shape *child)

Clearing the review request; ping me on IRC if you post a new patch and I'll review speedily.

(There is an RAII way to do what PropertyTree::insertChild and KidsChunk::create are doing here. Not right this instant, perhaps. But someday.)

Comment 4

[timeless]

Attachment: handle hash->add failure in insertChild

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/5dc38da16e79

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5dc38da16e79