898608 - Drive static analysis of unsafe references to zero

Status: NEW

(Core :: JavaScript: GC, task)

Comment 1

[Terrence Cole [:terrence]]

This probably shouldn't block us from closing the ExactRooting bug. That ship sailed a /long/ time ago.

Comment 2

[Paul Bone [:pbone]]

A list of unsafe references is here:

https://queue.taskcluster.net/v1/task/UoR8Wg1oQcO9QtIRx_DcQw/runs/0/artifacts/public/build/refs.txt.gz

Does this file get updated as these are fixed or how do we find a new path to an updated file?

Comment 3

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

(In reply to Paul Bone [:pbone] from comment #2)
> A list of unsafe references is here:
> 
> https://queue.taskcluster.net/v1/task/UoR8Wg1oQcO9QtIRx_DcQw/runs/0/
> artifacts/public/build/refs.txt.gz
> 
> Does this file get updated as these are fixed or how do we find a new path
> to an updated file?

That url is for a specific build. Here's one that always points to the latest results for mozilla-central:

https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.latest.firefox.browser-haz-debug/artifacts/public/build/refs.txt.gz

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Are the issues reported in refs.txt still worth fixing? I don't understand all of what's reported. Eg.

Function '_ZN26DynamicImportContextObject23clearReferencingPrivateEP9JSRuntimePS_$void DynamicImportContextObject::clearReferencingPrivate(JSRuntime*, DynamicImportContextObject*)' takes unsafe address of unrooted 'value' at js/src/vm/Modules.cpp:2847

AFAICT the complaint here is that we're passing a value by reference, but the callback is being called doesn't GC, so I'm not entirely sure what we're trying to eliminate under this bug.

Comment 5

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

They're definitely not a list of things to fix, since it's a very very conservative check. The idea is to warn about any time a pointer to a GC pointer is created, unless it's stored in a Handle. Consider:

void f(JSContext* cx) {
  Value v = createValue();
  Value* vp = &v; // warn here
  v = UndefinedValue();
  JS_MaybeCollectGarbageForFun(cx); // v is statically known to not hold a GC pointer; no hazard reported.
  MaybeGrabAValue(vp); // This *might* assign to *vp.
  JS_MaybeCollectGarbageForFun(cx); // is *vp rooted? We don't know. No hazard reported.
  MaybeGrabAValueRef(v); // Pass by reference -- same problem. Warn here.
  JS_MaybeCollectGarbageForFun(cx); // v might have been assigned to, but no hazard reported.
}

Come to think of it, I'm not sure the MaybeGrabAValue(vp) case will issue a warning anywhere. So this check has lots of both false positives and false negatives.

However, the specific example you point to could be cleaned up, and probably throw out a lot of the false alarms. JS::Value has to be passed by reference, but a const reference is safe since it can't be assigned to. Historically, the analysis had access to neither const qualifiers nor references (as in, it couldn't distinguish references from pointers). It definitely has the latter now, and I think it stores constness at this point? I'd have to look; I haven't touched the analysis in quite a while.

...looking...

Ok, the analysis doesn't see the const. Or more specifically: passing value there creates a non-const reference and then passes it to a const reference parameter. So to handle this the analysis would have to know that the parameter type is const. And it doesn't, though it could: the demangled function that it has access to is void JSRuntime::releaseScriptPrivate(JS::Value&), sans const, but the mangled name is _ZN9JSRuntime20releaseScriptPrivateERKN2JS5ValueE which does include the const-ness. It's the K:

% c++filt _ZN9JSRuntime20releaseScriptPrivateERKN2JS5ValueE _ZN9JSRuntime20releaseScriptPrivateERN2JS5ValueE  
JSRuntime::releaseScriptPrivate(JS::Value const&)
JSRuntime::releaseScriptPrivate(JS::Value&)

So the logic would be: if an address of a GC pointer type is taken to produce a reference, and then is only used as a const reference, then do not warn. (And I'm not sure the "...to produce a reference" part matters; isn't passing something as Value* const safe too?) Oh! Except that if the called function does GC, it's back to being unsafe. So add that in. And technically, there should be an escape analysis as well:

Value* evil;
void store(const Value& innocuous) {
  evil = &innocuous;
}
void f(JSContext* cx, Handle<Value> v) {
  store(v); // v converted from Handle<Value> → Value& → const Value& and store() does not GC. Safe!
  JS_GarbageCollectForFunAndProfit(cx); // Oops.
}

tl;dr: it's complicated.

Comment 6

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

Oh. I guess &innocuous would produce a, uh, const Value*? Or is it Value * const? Either way, that wouldn't compile due to const-correctness. But that doesn't really change much; you could store it into a Value const * const and cause the same problem.

Comment 7

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

I'm not sure if this bug has value as a meta bug. Though if you look at the closed bugs that this depends on, they're valid changes moving in the direction of safety. And theoretically this bug could also depend on analysis improvements to reduce the number of false alarms.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

yeah, that was really going to be my next question: Is it worth pushing on this (or maybe building out a series of bugs for contributors?) or is this something that we should just look at when we need to look at it?

Comment 9

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

It's not a bad source of small bugs for contributors, though it's not a great one either. (You have to understand quite a bit in order to know what to change and whether it really makes sense to change it.) With some work, it could generate some good-first-bugs, but it would require someone knowledgeable to read through the list and do the analysis (so, 90% of the effort).

If I were to take this seriously, I would probably:

• Extract from the list the places where we could use a Handle or MutableHandle and fix those (or make bugs for them)
• Identify patterns where some annotation or modified code pattern would eliminate the warnings (to reduce noise)
• Categorize the remaining warnings (and automate the categorization where possible, to add to the report)

Honestly, though, I'm not going to get to it. I still want to leave this bug open, because it's valid and improvements here really would make some amount of difference. (When I get weird GC crashes, I sometimes look at this report to see if there's something related.)

I don't think it needs to be a meta bug right now. In the event that I or somebody starts looking into it, then it might make sense to re-meta it depending on what bugs are filed.