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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

({coverity})

Trunk
x86
All
coverity
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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);
(Assignee)

Comment 1

7 years ago
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
(Assignee)

Comment 2

7 years ago
Created attachment 493530 [details] [diff] [review]
patch
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)
(Assignee)

Updated

7 years ago
Summary: PropertyTree::getChild returns without unlocking cx->runtime when newShape or insertChild fail → PropertyTree::insertChild returns without unlocking cx->runtime when hash->add fails
(Assignee)

Comment 4

7 years ago
Created attachment 493934 [details] [diff] [review]
handle hash->add failure in insertChild
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/tracemonkey/rev/5dc38da16e79
Whiteboard: [fixed-in-tracemonkey]

Comment 6

7 years ago
http://hg.mozilla.org/mozilla-central/rev/5dc38da16e79
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Attachment #493934 - Flags: approval2.0?
You need to log in before you can comment on or make changes to this bug.