Open Bug 877537 Opened 11 years ago Updated 2 years ago

Possible hole in rooting analysis with Value[] on stack

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

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....
Assignee: general → nobody
Steve, is this likely to still be a problem?
Flags: needinfo?(sphink)
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]?
Flags: needinfo?(sphink)
> 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.
Ah! Ok. I'll write a test case for that, then, and see what it reports. Thanks!
Flags: needinfo?(sphink)
Blocks: hazard
Flags: needinfo?(sphink)
(Note that this is probably already fixed. For a long time the analysis didn't understand arrays on the stack.)
Flags: needinfo?(sphink)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.