Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090302 Minefield/3.2a1pre
Build Identifier: 

 Overview of problem, provided by Taras
````````````````````````````````````````
nsCOMPtr is our reference counting pointer wrapper. A common use case is to manipulate an object while it's wrapped in nsCOMPtr<>, then use the swap call to return it (while avoiding reference counting).

So an example is
nsCOMPtr<nsIFoo> foo; // at this point full contains a null-ed pointer
foo = ....
foo->doStuff()
nsIFoo *foo_to_return;//this could also be a parameter
foo.swap(foo_to_return);
// now foo's pointer is pointing to garbage
....
if (something)
 return foo_to_return;
foo->doStuff();//this is a bug,

It'd be nice to have an analysis to ensure that we do not have dangling
pointers due to swap().

 Overview of analysis
``````````````````````
The analysis cares about two values: GARBAGE and NULL.  "GARBAGE" is the value that not-necessarily-initialized memory has; e.g., what outparams point to, what stack-allocated pointers point to, etc.  The analysis warns when (1) GARBAGE is "used" by an expression; and (2) NULL is dereferenced.

The analysis proceeds by first locating calls to nsCOMPtr::swap(this, a).  If any exist in a function F, the analysis then finds the set V of variables in all the backward slices from each swap() call site.  Next, the analysis abstractly interprets F using esp --- the set V is used as the "property variables" in esp's framework.  Errors are reported as desribed above.

This analysis is superficially fairly similar to the 'outparams' analysis.  This analysis is also unsound.

 Overview of implementation
````````````````````````````
The analysis (PointerSwapCheck) inherits from ESP.Analysis and does a rather straightforward abstract interpretation.  The semantics of the nsCOMPtr methods are provided via a "mix-in."  At each call site, an abstract interpreter of the function being called is dispatched by PointerSwapCheck.interpCall().  The COMPtr mixin overrides imporant methods in this manner.  The idea is to make it easier to add interpreters for other smart pointers.

A lot of code has been borrowed from the outparams analysis: specifically, the code that deciphers the semantics of function parameters.

 Questions for reviewer(s)
```````````````````````````
 1.) Suggestions on how/where to share code with outparams?
 2.) Any functions in ptr-swap-check that could be promoted to a library?
 3.) Should it be an error to swap() with garbage, in addition to being an error to use garbage?  (Currently, this is *not* reported.)
 4.) Is it an error to destroy an nsCOMPtr that contains a garbage pointer?  (Currently, this *is* reported.)

The patches to get this running, and initial results, will be supplied soon.

Reproducible: Always

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 3 • 16 years ago

From a spot check, it appears that many of the "may dereference NULL" warnings are occurring because getter_AddRefs() is not being handled correctly.  Specifically, it appears that its argument is being marked as having INPARAM semantics, instead of INOUTPARAM (?), which means that the argument is not being set to TOP during interpretation of getter_AddRefs().  David, if I altered your func_param_sems to handle this case correctly, would it break outparams?

As for the rest of the warnings, please advise on who is best qualified to sort through them.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 4 • 16 years ago

RE comment 3:

These are merely the warnings reported by the swap analysis.  g++ additionally printed a crapload more.  If anyone wants the full list, please let me know.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 5 • 16 years ago

(In reply to comment #0)
> 3.) Should it be an error to swap() with garbage

I guess it's possible that someone could know what they're doing when they swap in garbage, but any further assignment besides swap will call Release on garbage and that's really bad. I'd report.

> in addition to being an error to use garbage?

Most definitely.

>  4.) Is it an error to destroy an nsCOMPtr that contains a garbage pointer?

Yes. Destruction calls Release on mRawPtr if it is non-null, and that's bad.

	David Mandelin [:dmandelin]
	Comment 6 • 16 years ago

Initial comments:

- You don't necessarily need any ESP property variables. And if you do, you might just need the variables that are referred to in calls to swap. The analysis scales better if you use fewer property variables, so you probably want to try to cut down.

- The lattice looks a bit strange to me. In particular, GARBAGE is not strictly necessary and could generally be replaced by TOP, while at the same time, you need a 'not-garbage value'. I would suggest using the abstract values VALID (points to a valid object), NULL (null pointer) and TOP. When you take the address of something or get a value through an outparam it is VALID. With this lattice, the checker should require:

  - the target of a dereference operation is always VALID

  - a value returned or assigned to an outparam is always VALID or NULL

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 7 • 16 years ago

Update: I'm going to address David's comments today.

Until the second version of ptr-swap-check is ready, I'm going to split off the xhydra bugfixes in attachment 2 [details] [diff] [review] into a separate bug.  Though ptr-swap-check depends on them, all the fixes are general (though the generality of one modification of esp is debatable.)

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 8 • 16 years ago

RE comment 6:

I agree with your point about the strange lattice; the analysis you describe is close to what I originally had in mind.  However, I guessed that there would be a huge number of false positives from this type of analysis, and thus prioritized precision over soundness: the analysis I described is egregiously unsound for that reason.  It's in some sense the complement of what you suggest.  HOWEVER, I never checked my assumption about there being too many false positives.

It appears to be relatively easy to switch ptr-swap-check to the analysis described in comment 6.  I'll do that and see what warnings I get.

	(dormant account)
	Comment 9 • 16 years ago

Comment on attachment 366455 [details]
Results of analysis (v1)

>In file included from <command line>:2:
>/home/higgins/mozilla/mozilla-central/xpcom/glue/nsThreadUtils.cpp: In function ânsresult NS_NewThread(nsIThread**, nsIRunnable*)â:
>/home/higgins/mozilla/mozilla-central/xpcom/glue/nsThreadUtils.cpp:82: warning: 'thread' may dereference NULL

That's not actually an issue, nulls are fine. We are worried about swap()ing uninitialized memory.

>In file included from <command line>:2:
>/home/higgins/mozilla/mozilla-central/netwerk/base/public/nsNetUtil.h: In function ânsresult NS_DoImplGetInnermostURI(nsINestedURI*, nsIURI**)â:
>/home/higgins/mozilla/mozilla-central/netwerk/base/public/nsNetUtil.h:1304: warning: '(expression)' may evaluate to garbage
>/home/higgins/mozilla/mozilla-central/netwerk/base/public/nsNetUtil.h:1322: warning: possible free/unref of garbage

This is a false positive.

 NS_PRECONDITION(!*result, "Must have null *result");

I think it's time for the time-honoured tradition of inferring values via asserts.

	(dormant account)
	Comment 10 • 16 years ago

Also it should be ok to swap in garbage if the nsCOMPtr is not used after that point. Ie if it's a local variable that didn't escape.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 11 • 16 years ago

RE comment 9:

(1) This warning actually appeared because the analysis didn't correctly handle getter_AddRefs, namely that when it is used to decorate an argument to a function call, the argument becomes a sort of "transitive outparam."  This has been fixed in the most recent version of ptr-swap-check (not attached), and the warning you refer to is suppressed.

About the 'NULL derefence' warning in general: after our e-mail exchange, I realized that the ptr-swap analysis could check for NULL-derefs basically for free, as a side effect of the swap() analysis.  That's why this type of warning shows up.

(2) Yeah, this is not a fun one ;).  It's an error according to the "spec", because a pointer contained by an outparam is dereferenced before being set to a safe value.  Has anyone worked on "decompiling" some of these useful macros for xhydra?  This is a bit more than I want to tackle at the moment.  If not, for the time being I'd rather resort to "whitelisting" this function.  The machinery is already in place.  Is this suitable? 

Another option is to get treehydra or esp to understand the control flow of 'DebugBreak', 'assert', etc.  That way, all our path-sensitive analyses that refine values based on path conditions will eliminate these types of false positives automagically.


RE comment 10:

OK.  Yesterday, I added a flag to enable/disable these warnings.  I'll leave them disabled for now.

	(dormant account)
	Comment 12 • 16 years ago

> 
> (2) Yeah, this is not a fun one ;).  It's an error according to the "spec",
> because a pointer contained by an outparam is dereferenced before being set to
> a safe value.  Has anyone worked on "decompiling" some of these useful macros
> for xhydra?  This is a bit more than I want to tackle at the moment.  If not,
> for the time being I'd rather resort to "whitelisting" this function.  The
> machinery is already in place.  Is this suitable? 

Is that just some fun points-to analysis? :)

So how about hacking around this. Dave already has some code to detect outparams, you can just assume that assignments to outparams happen through *..ie just ignore the dererence as much as you can. That may or may not get you enough.

> 
> Another option is to get treehydra or esp to understand the control flow of
> 'DebugBreak', 'assert', etc.  That way, all our path-sensitive analyses that
> refine values based on path conditions will eliminate these types of false
> positives automagically.

Yeah I'm just suggesting that we teach the analysis framework(zerononzero part?) about a couple of assert macros. We can even modify what the assert macro expands to to make it easier to match on.


btw, congrads on getting this working, I'm impressed.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 13 • 16 years ago

>> (2) Yeah, this is not a fun one ;).  It's an error according to the "spec",
>> because a pointer contained by an outparam is dereferenced before being set to
>> a safe value.  Has anyone worked on "decompiling" some of these useful macros
>> for xhydra?  This is a bit more than I want to tackle at the moment.  If not,
>> for the time being I'd rather resort to "whitelisting" this function.  The
>> machinery is already in place.  Is this suitable? 
> 
> Is that just some fun points-to analysis? :)
> 

Oh boy --- "points-to analysis" is a phrase I'd hoped we could avoid in this bug ;).

> So how about hacking around this. Dave already has some code to detect
> outparams, you can just assume that assignments to outparams happen through
> *..ie just ignore the dererence as much as you can. That may or may not get you
> enough.
> 

OK.  I can try to hack in some code to ignore deref's of outparams in locations that are "probably OK," like right at the beginning of long functions.

>> Another option is to get treehydra or esp to understand the control flow of
>> 'DebugBreak', 'assert', etc.  That way, all our path-sensitive analyses that
>> refine values based on path conditions will eliminate these types of false
>> positives automagically.
> 
> Yeah I'm just suggesting that we teach the analysis framework(zerononzero
> part?) about a couple of assert macros. We can even modify what the assert
> macro expands to to make it easier to match on.
> 

I just looked at the glibc 'assert(C)' macro, and it expands into code that calls an '__attribute__((__noreturn__))' function when 'C' is false.  If gcc already modifies the CFG appropriately for calls to "noreturn" functions, then this looks like all we need.  A quick inspection of the XPCOM debugging macros suggests that they need to refactored a bit to allow adding this attribute (and I'm probably not the guy to do it yet), but if that's done, then I think we'd be set.  That should hopefully be less painful than trying to decompile macros.  At the very least, this attribute could be added for the static analysis builds.

	(dormant account)
	Comment 14 • 16 years ago

> OK.  I can try to hack in some code to ignore deref's of outparams in locations
> that are "probably OK," like right at the beginning of long functions.

should make dereferencing outparams always ok afaik. To clarify, I meant 
*outparam = foo or assert(*outparam) should just end up with * being skipped and pretending that "outparam" is used to directly.

> I just looked at the glibc 'assert(C)' macro, and it expands into code that
> calls an '__attribute__((__noreturn__))' function when 'C' is false.  If gcc
> already modifies the CFG appropriately for calls to "noreturn" functions, then
> this looks like all we need.  A quick inspection of the XPCOM debugging macros
> suggests that they need to refactored a bit to allow adding this attribute (and
> I'm probably not the guy to do it yet), but if that's done, then I think we'd
> be set.  That should hopefully be less painful than trying to decompile macros.
>  At the very least, this attribute could be added for the static analysis
> builds.

Heh, fyi mozilla asserts are soft. So they aren't noreturn. It's easy to add whatever attributes/inline functions need. See 
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsError.h#115 for an example of how to special-case static analysis code.

Redefine whatever assert macros you need. It's reasonable to modify the codebase to ease static analysis.

	David Mandelin [:dmandelin]
	Comment 15 • 16 years ago

>> Another option is to get treehydra or esp to understand the control flow of
>> 'DebugBreak', 'assert', etc.  That way, all our path-sensitive analyses that
>> refine values based on path conditions will eliminate these types of false
>> positives automagically.

1. Yes, this is the way to go. But we need to be careful about definitions in order to avoid hiding bugs. Classically, there are two directives used to guide static analyses:

    assert(e) - The analysis should report an error if e may be false.

    assume(e) - The analysis should analyze only the cases where e is true.
                Informally, the analysis should update its state so that e
                is true.

A C/C++ assert-type macro could represent either directive, or neither. It seems that for this problem we want NS_PRECONDITION to mean 'assume', and other assert macros to mean neither (we aren't checking random assertions, just the invalid-pointer-dereference property). As Taras suggested, redefining NS_PRECONDITION to call a function you create is a good way to implement this.

Because macros are macros, NS_PRECONDITION(e) is going to compile to:

    bool __v = e;
    if (__v)
        __assumeFalse();

and __assumeFalse can be implemented easily. You could also turn NS_PRECONDITION into a function with 'assume' semantics, which has a slightly more complex implementation. It could have some advantages, but it's always nice to start with the dumb way.

2. You might get some false positives that can be solved by adding more NS_PRECONDITIONs. If so, we probably want to add them, but we should check with bsmedberg on how far we want to go there.

3. Usually, when an analysis uses preconditions, it should try to verify them at call sites as well. I guess that's less important here because we will have assertions in debug builds, but we should probably try it out when you're done with the main part.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 16 • 16 years ago

>> OK.  I can try to hack in some code to ignore deref's of outparams in locations
>> that are "probably OK," like right at the beginning of long functions.
> 
> should make dereferencing outparams always ok afaik. To clarify, I meant 
> *outparam = foo or assert(*outparam) should just end up with * being skipped
> and pretending that "outparam" is used to directly.
> 

Oh --- I misunderstood you.  Yes, the analysis is doing what you describe.  I was proposing a dirty hack to suppress this special (and OK) kind of outparam use-before-def warning.

But the developing consensus, towards annotating assert/assume statements, is definitely preferable to this hack.  I very much like what David proposes in comment 15.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 17 • 16 years ago

(In reply to comment #10)
> Also it should be ok to swap in garbage if the nsCOMPtr is not used after that
> point. Ie if it's a local variable that didn't escape.

The nsCOMPtr destructor calls Release on its ptr if non-null which would crash if garbage was swapped in.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 18 • 16 years ago

Assigning to me, although I may get back to working on this for a while.