[feature request] null pointer check after dereference

[Quuxplusone]

Bugzilla Link	PR30232
Status	NEW
Importance	P enhancement
Reported by	Gonzalo BG (gonzalo.gadeschi@gmail.com)
Reported on	2016-09-01 06:48:48 -0700
Last modified on	2019-04-30 03:49:48 -0700
Version	unspecified
Hardware	All All
CC	alexfh@google.com, arturocampos82@gmail.com, danniwezz@hotmail.com, development@jonas-toth.eu, djasper@google.com, eugene.zelenko@gmail.com, ganna@apple.com, ki.stfu@gmail.com, klimek@google.com, llvm-dev@redking.me.uk, piotr.padlewski@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

I think that a function-scope check that warns on null pointer tests after
dereferencing would be very useful.

For example, in the following snippet:

int* foo();
void baz();

void bar() {
  int* a = foo();
  a[0] = 2;
  if(a)
    baz();
}

a is used before the test that check if its a null pointer:

- if foo() can return a null pointer, the check should come before the
assignment. - if it cannot, then the check is unnecessary.

In both cases, there is always something suspicious going on and either clang
or clang-tidy should warn about it (clang would be preferable).

Note, in real examples, the dereference of a before the nullpointer check can
happened behind a macro, or inside an intermediate function.

A good place to start would be to restrict such checks to a function scope
first, which should catch issues inside macros, but omit following every
function call that can access the pointer in the analysis.

If desired, doing that can be done a posteriory, but even if it is found that a
function validates the pointer, then a second validation after it becomes
unnecessary, so the check should still warn about it.

[Quuxplusone]

Doesn't Static Analyzer core.NullDereference do this?

[Quuxplusone]

If I remember corectly

int *a = foo();
*a = 42;
if (!a) { something  }

will be found with DeadCode checker

but I guess it won't gonna find your case.

[Quuxplusone]

We cannot add a path-sensitive analyzer check for this because the check requires checking a property over all paths, but the analyzer does not guarantee that it will cover all paths.

This can be implemented as a flow-sensitive analysis on top of clang's CFG. An example of another data-flow analysis is LiveVariables.

This could be a compiler warning. There is no reason to restrict the check to clang-tidy users only.

[Quuxplusone]

Arturo Campos declared to work on it, so I temporarily assign myself to it (until Arturo will have access to bugzilla)

[Quuxplusone]

Hi,

Im looking to find a project for GSoC and found this on their website as a good project to do for the summer. Is it still available and if so would you like someone to have this project as their job this summer?

[Quuxplusone]

It is still open, so feel free to implement it :)

[Quuxplusone]

Hi,

i think such a check can be could implement cppcoreguidelines gsl::not_null<T*> semantics. Every pointer, that is not checked before dereferencing should be declared as not_null.

Redundant checks are then easily spottable.

What do you think about that +Daniel Posch, would you implement this?

[Quuxplusone]

https://www.viva64.com/en/b/0629/

The recent PVS Studio check of the llvm/clang source found a LOT of these
"check a pointer after dereferencing" cases - getting even basic detection for
this into the clang tools would be very useful.

int testnull(int *p);
int testnull(int *p) {
    int value = *p;
    return p ? value : 0;
}

https://godbolt.org/z/B7-cZv