877537 - Possible hole in rooting analysis with Value[] on stack

Status: ASSIGNED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider this code from TestShellParent.cpp:

103   JS::Value argv[] = { STRING_TO_JSVAL(str) };
104   unsigned argc = ArrayLength(argv);
105 
106   JS::Value rval;
107   JSBool ok = JS_CallFunctionValue(mCx, global, mCallback, argc, argv, &rval);

The analysis warns about rval, but says nothing about argv....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Steve, is this likely to still be a problem?

Comment 2

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

Hm, why don't I remember this bug?

Is there further code below that? I don't see anything like that in the current TestShellParent.cpp. And the above snippet doesn't have a hazard -- while argv *is* directly on the static, and the analysis knows about that, its live range dies at the function entry.

Of course, if within the function call argv[0] gets relocated, you have a hazard. But that function call just sees a pointer to a pointer (well, a pointer to a Value, here). And there are a lot of places that pass around pointers to GC pointers. Most of them are safe -- usually because they're pointers to rooted memory anyway. So we have a separate report on anything that takes the address of an unrooted location. It has too many false positives to be used as a stick to keep things safe, but we do check it periodically. But it should have listed the above.

Unless there *was* some code after the above, code that used argv[0]?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I don't see anything like that in the current TestShellParent.cpp.

Well, sure.  I changed argv to use a .address() on a Rooted when I filed this bug, and then it got changed to a HandleValueArray at some point.  So the hazard in this code is long-gone no matterwhat.

The code as of when I filed the bug looked like http://hg.mozilla.org/mozilla-central/file/ccddc9279bae/ipc/testshell/TestShellParent.cpp#l103

There was no useful code after the JS_CallFunctionValue here.  Both rval and argv were unused after that call.

What I was worried about when I filed this bug (as best as I can recall; it's been 2+ years) was that the "&rval" was being reported as an unsafe reference (because it takes the address of an unrooted location).  But at least at the time "argv" was not being reported as an unsafe reference.  That's what the bug was about.

Comment 4

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

Ah! Ok. I'll write a test case for that, then, and see what it reports. Thanks!

Comment 5

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

(Note that this is probably already fixed. For a long time the analysis didn't understand arrays on the stack.)

Comment 6

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

Added a test.

Comment 7

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

Attachment: Bug 877537 - Verify that on-stack Cell*[] arrays are checked