877543 - Hole in rooting analysis with bizzare casting

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

From JSObjectBuilder.cpp:

void
JSObjectBuilder::ArrayPush(JSCustomArray *aArray, JSCustomObject *aObject)
{
  if (!mOk)
    return;

  uint32_t length;
  mOk = JS_GetArrayLength(mCx, (JSObject*)aArray, &length);

  if (!mOk)
    return;

  JS::Value objval = OBJECT_TO_JSVAL((JSObject*)aObject);
  mOk = JS_SetElement(mCx, (JSObject*)aArray, length, &objval);
}

If aArray is really a JSObject*, then it's unrooted across the JS_GetArrayLength call, which can definitely GC...

Comment 1

[Brian Hackett [Laid off!]]

The static rooting analysis is built on the assumption that the types in the program aren't lies.  Unfortunately the sixgill frontend doesn't make it easy to detect casts, so the best way to catch this sort of behavior is with manually grepping.

Comment 2

[Boris Zbarsky [:bzbarsky]]

OK, makes sense.  I wasn't necessarily expecting there to be a sane way to catch this, just filed it in case.

I filed bug 877658 on fixing that code.

Comment 3

[Steve Fink [:sfink] [:s:]]

Note that this issue was resolved by making just about everything take Handle<>s instead of bare pointers, so now the original code would not compile (can't cast JSObect* to Handle<JSObject*>).