[cfe-dev] bug with NonNullParamChecker?

[Jordan Rose]

Hi, Mathieu. You've run into one of the analyzer's haziest features: false positive suppression based on return values. To put it simply, if there's a null pointer dereference, and it turns out the null pointer came from an inlined function, we suppress the warning.

Why? Well, for better or for worse the analyzer does not have perfect information. Consider an example like this:

	targetMap[targetName]->process(input);

Seems harmless, right? Well, what does the map's operator[] look like?

	Target *getItem(StringRef name) {
		if (map contains a target for name)
			return it
		return NULL;
	}

Most likely, the analyzer doesn't have perfect knowledge about the contents of the map, so it assumes both branches can be taken—which is totally reasonable! However, the caller could very well have some extra knowledge: the targetName is checked at the beginning of the program, and so the map lookup will never fail at this point.

Usually, the response to this sort of bug is to add an assertion, but in this case you wouldn't be able to do that without introducing a temporary variable and breaking apart the original expression.

When we first turned on C++ inlining, this sort of example resulting in hundreds of false posiitves on the LLVM code base. It did find some true positives, such as unchecked calls to dyn_cast, but in general it was just not compatible with the way people write code. A lot of times, generic functions have to handle error cases that the caller knows won't happen, but would find annoying or difficult to actually assert() about. We've found that a lot of these cases correspond to a null value being returned, which is entirely a heuristic.

The false positive suppression heuristics can definitely be improved, but in general false negatives are better for the analyzer than false positives. The former reduces bugs caught and confidence in the tool, but the latter results in people turning off a checker wholesale or not running the analyzer at all.

Anyway, thanks for the report, and if you come up with any "counter-suppression" ideas, please send them to the list!

Jordan


On May 1, 2013, at 22:16 , Mathieu Baudet <mathieubaudet at fb.com> wrote:

> Hi,
> 
> As I was testing NonNullParamChecker this afternoon, I ran into this troubling example:
> 
> // --------- example 1 ------------
> void *getNull() {
>  return 0;
> }
> 
> void check(void *p) __attribute__(( nonnull ));
> void check(void *p) {
> }
> 
> int main(int argc, char **argv) {
>  void *p = getNull();
>  check(p);
>  return 0;
> }
> // --------------------------------
> 
> This code gives no warning on the versions of clang that I could test:
> - Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)
> - clang version 3.3 (trunk 180768)
> - clang version 3.3 (trunk 180907) (llvm/trunk 180768)
> 
> To get an error one I have to replace p = getNull() by p = 0.
> 
> First I was tempted to think it was just a limitation of the core analyzer, but
> 1) I obtain an error with a similar example where the nonnull attribute is replaced by a division by zero (see example 2 at the end)
> 
> 2) I debugged the file NonNullParamChecker.cpp : I am very new to this codebase but it seems that a report is actually emitted (lines 119-139). Then it never shows up for some reason...
> 
> Is this a bug? If not, how can we improve this checker?
> 
> Thanks!
> --
> Mathieu
> 
> 
> // --------- example 2 -----------
> int getX() {
>  return 0;
> }
> 
> void check(int p) {
>  1 / p;
> }
> 
> int main(int argc, char **argv) {
>  int x = getX();
>  check(x);
>  return 0;
> }
> 
> 
> _______________________________________________
> 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/20130502/b5e10467/attachment.html>