Closed
Bug 643444
Opened 13 years ago
Closed 12 years ago
Investigate the callers of js_GetPropertyHelperWithShape() for hazards
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: igor, Assigned: igor)
References
Details
js_GetPropertyHelperWithShape introduced in the bug 596026 returns to the caller the instance of Shape. Hover GetPropertyByName, that uses the shape result, apparently does not check that the returned shape still exists in the object and update the pic with the shape's slot even if the property was removed in the getter. We should investigate if this can lead to a hazard.
Comment 1•13 years ago
|
||
To me it looks like the PICTable is sound if the shape guarantees are sound.
It should have an assertion, though, something like:
jstracer.cpp
> if (picTable->scan(obj->shape(), id, &slot)) {
>+ JS_ASSERT(obj->nativeLookup(id)->slot == slot);
> *vp = obj->getSlot(slot);
> return WasBuiltinSuccessful(tm);
> }
We could further assert that the property has the default getter.
Assignee | ||
Comment 2•13 years ago
|
||
GetPropertyByName contains: /* Try to hit in the cache. */ uint32 slot; if (picTable->scan(obj->shape(), id, &slot)) { *vp = obj->getSlot(slot); return WasBuiltinSuccessful(tm); } const Shape *shape; JSObject *holder; if (!js_GetPropertyHelperWithShape(cx, obj, obj, id, JSGET_METHOD_BARRIER, vp, &shape, &holder)) { SetBuiltinError(tm); return false; } /* Only update the table when the object is the holder of the property. */ if (obj == holder && shape->hasSlot() && shape->hasDefaultGetter()) { /* * Note: we insert the non-normalized id into the table so you don't need to * normalize it before hitting in the table (faster lookup). */ picTable->update(obj->shape(), id, shape->slot); } So if the property is not in the cache, then js_GetPropertyHelperWithShape is called. Then, if the shape has a getter, that getter can remove the shape. There is no GC hazard even without GC-allocated shapes. The shape is explicitly rooted during the getter invocation and there is no possibility of the GC until shape->slot is accessed. But this is very fragile. It would be better IMO to call picTable->update directly from js_GetPropertyHelperWithShape.
Comment 3•13 years ago
|
||
(In reply to comment #2) > So if the property is not in the cache, then js_GetPropertyHelperWithShape is > called. Then, if the shape has a getter, that getter can remove the shape. > There is no GC hazard even without GC-allocated shapes. The shape is explicitly > rooted during the getter invocation and there is no possibility of the GC until > shape->slot is accessed. Oh, I see what you're worried about. > But this is very fragile. It would be better IMO to call picTable->update > directly from js_GetPropertyHelperWithShape. Or pass an AutoShapeRooter reference instead of an out pointer, and add an assign() method to AutoShapeRooter. Or make shapes normal GC-things so the conservative scanner sees shape pointers. Can we open this up, since it seems there's no security hole?
Comment 4•13 years ago
|
||
(In reply to comment #3) > Or pass an AutoShapeRooter reference I mean pointer.
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to comment #3) > Can we open this up, since it seems there's no security hole? Done.
Group: core-security
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•