[PATCH] D120992: [analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp:166
+/// child is a sink node.
+static bool unconditionallyLeadsHere(const ExplodedNode *N) {
+  size_t NonSinkNodeCount = llvm::count_if(
----------------
NoQ wrote:
> xazax.hun wrote:
> > Szelethus wrote:
> > > xazax.hun wrote:
> > > > xazax.hun wrote:
> > > > > Consider the following code snippet:
> > > > > ```
> > > > > void f(int *p, bool b)
> > > > > {
> > > > >   if (b) {
> > > > >     *p = 4;
> > > > >   }
> > > > >   if (p) {
> > > > >    ...
> > > > >   }
> > > > > }
> > > > > ```
> > > > > 
> > > > > I suspect that we would get a warning for the code above. I think warning on the code above might be reasonable (the values of `b` and `p` might be correlated but in some cases the analyzer has no way to know this, probably some assertions could make the code clearer in that case).
> > > > > 
> > > > > My problem is with the wording of the error message.
> > > > > The warning `Pointer is unconditionally non-null here` on the null check is not true for the code above.
> > > > Also, if the check would warn for the code snippet above, the note "suggest moving the condition here" would also be incorrect.
> > > What if we demand that the the `CFGBlock` of the dereference must dominate the `CFGBlock` of the condition point?
> > I think it makes sense to warn both when the dereference dominates the null check, and when the null check post-dominates the dereference. We just want to give different error messages in those cases. 
> > What if we demand that the the CFGBlock of the dereference must dominate the CFGBlock of the condition point?
> 
> ```lang=c
>   *p = 4;
>   if (b) {
>     p = bar();
>   }
>   if (p) {
>     ...
>   }
> ```
> 
Yup, this is a nice example. I cannot think of an easy way around this using symbolic execution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120992/new/

https://reviews.llvm.org/D120992