Closed Bug 643444 Opened 13 years ago Closed 12 years ago

Investigate the callers of js_GetPropertyHelperWithShape() for hazards

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

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.
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.
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.
(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?
(In reply to comment #3)
> Or pass an AutoShapeRooter reference

I mean pointer.
(In reply to comment #3)
> Can we open this up, since it seems there's no security hole?

Done.
Group: core-security
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.