[cfe-dev] Static Analyzer: NullDereference checker report not visible

[Jordan Rose]

You've discovered a bit of our false positive suppression. Consider this slightly different case:

Foo *getFooForKey(FooMap &map, const char *key) {
  FooMapIter iter = map.find(key);
  if (iter.isInvalid())
    return 0;
  assert(iter.value);
  return iter.value;
}

void processMap(FooMap &map) {
  std::vector<const char *> keys = map.getKeys();
  filterMapKeys(keys);
  for (std::vector<const char *>::const_iterator I = keys.begin(), E = keys.end(); I != E; ++I) {
    getFooForKey(map, *I)->processed = true; // ***
  }
}

In processMap, we can be sure that (barring multithreading) all the keys are present in the map and we'll never get a null pointer value back. But there's no real way for the analyzer to know that, and so the only way to silence the error on the line marked *** would be to introduce a temporary variable, assert that it's non-null, and then use it. That's getting to the point where people would probably stop using the analyzer rather than change their code, especially in more complicated cases.

So, we added a heuristic that says "a null pointer returned from an inlined function is some kind of error path, and we should assume it doesn't happen in practice". This immediately knocked hundreds of map-lookup-style false positives off of the LLVM results. It does miss true bugs, as you've demonstrated, but currently it's hard to tell when a value is tested for null when it's already known to be null, because the state doesn't change. We've also considered adding a new function attribute that says "this function can return null in normal situations, so check it"; the advantage of such an attribute is that it would work on function declarations without definitions. (The disadvantage, of course, is that it's an extra non-standard annotation that most people won't use.)

Anyway, it's not great, but it's better than the alternative, even if less theoretically correct.
Jordan


On Jul 26, 2013, at 6:15 , Gábor Kozár <kozargabor at gmail.com> wrote:

> I'm playing around with the NullDereference checker (Dereference.cpp) in an attempt to understand how it works.
> 
> I've discovered a strange phenomenon. Sometimes the checker finds a bug, calls reportBug(), a BugReport is created and emitted -- but no warning is raised.
> My test code:
> 
> struct Foo
> {
>         int bar;
> };
> 
> Foo* getFooPtr(bool cond)
> {
>     return cond ? new Foo : nullptr;
> }
> 
> int main(int argc, const char** argv)
> {
>         Foo* fp = getFooPtr(argc % 2 == 1);
>         if( ! fp )
>         {
>                 fp->bar = 0; // bug!
>         }
> 
>         return 0;
> }
> 
> Interestingly enough, if I replace the getFooPtr definition above with this:
> 
> extern Foo* getFooPtr(bool);
> 
> Then the report becomes visible.
> 
> Note: I'm using Clang 3.3, but I also checked the latest SVN revision, and there don't seem to be any changes to the DereferenceChecker code.
> 
> Is this a bug, or why could this be? Thanks!
> 
> Gabor
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130730/f6e8d14c/attachment.html>