Closed Bug 614928 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

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);
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.
Summary: PropertyTree::getChild returns without unlocking cx->runtime when insertChild fails → PropertyTree::getChild returns without unlocking cx->runtime when newShape or insertChild fail
Attached patch patch (obsolete) — Splinter Review
Assignee: general → timeless
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #493530 - Flags: review?(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.)
Attachment #493530 - Flags: review?(jorendorff)
Summary: PropertyTree::getChild returns without unlocking cx->runtime when newShape or insertChild fail → PropertyTree::insertChild returns without unlocking cx->runtime when hash->add fails
Attachment #493530 - Attachment is obsolete: true
Attachment #493934 - Flags: review?(jorendorff)
Attachment #493934 - Flags: approval2.0?
Attachment #493934 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/mozilla-central/rev/5dc38da16e79
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #493934 - Flags: approval2.0?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: