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)
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)
814 bytes,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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
Assignee: general → timeless
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #493530 -
Flags: review?(jorendorff)
Comment 3•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #493934 -
Flags: review?(jorendorff) → review+
Comment 5•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/5dc38da16e79
Whiteboard: [fixed-in-tracemonkey]
Comment 6•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5dc38da16e79
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #493934 -
Flags: approval2.0?
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•