[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D97183#2650172 <https://reviews.llvm.org/D97183#2650172>, @steakhal wrote:

> In D97183#2640559 <https://reviews.llvm.org/D97183#2640559>, @RedDocMD wrote:
>
>> @NoQ, why does the following trigger a null-dereference warning? (https://godbolt.org/z/Kxox8qd16)
>>
>>   void g(std::unique_ptr<A> a) {
>>     A *aptr = a.get();
>>     if (!aptr) {}
>>     a->foo();
>>   }
>>
>> When `a->foo()` is called, the constraint `!aptr` is no longer valid and so `InnerPointerVal` corresponding to `a` is no longer constrained to be null.
>> Am I missing something?
>
> When the if's condition is evaluated, it probably triggered a state split. On one path the `aptr` (aka. the inner pointer) will be constrained to `null`.
> The only way to be sure is by checking the exploded graph and see where it goes.

Yes, that's, like, the whole point. We report unchecked use after a check. If the pointer is never null, why check? If the pointer is sometimes null, why use without a check? The code clearly doesn't make sense. That's what the report says: "assuming 'aptr' is null, there's null dereference on the next line". Once our simulation leaves the lexical scope of the if-condition 'aptr' doesn't suddenly become non-null. Technically speaking, we have no notion of a merge at all. There is literally no merge operation defined on our `ProgramState`, we do not perform fixpoint iteration, we do not try to combat path explosion, we simply simulate the behavior of the program on a few possible execution paths (defined by branching in the program) and report potential issues.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:106-109
+    if (const auto *DS = llvm::dyn_cast<DeclStmt>(S)) {
+      for (const Decl *D : DS->getDeclGroup()) {
+        if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) {
+          const Expr *Init = VD->getInit();
----------------
RedDocMD wrote:
> steakhal wrote:
> > RedDocMD wrote:
> > > steakhal wrote:
> > > > So you are trying to find the assignment, where the inner pointer is assigned to a variable.
> > > > This visitor logic seems to be somewhat convoluted.
> > > > 
> > > > What you want to achieve is slightly similar to `FindLastStoreBRVisitor`. You should have a look at that.
> > > That is what I had done before. @NoQ pointed out why this wouldn't work in a previous comment.
> > Please elaborate on that.
> > I'm not saying that an already existing visitor would perfectly fit your needs. I'm just curious why a //similar// logic would not work for you. You are trying to iterate over a bunch of decls and init exprs etc. And there is nothing similar to the visitor I mentioned.
> Sorry. I should have written it out better.
> > So you are trying to find the assignment, where the inner pointer is assigned to a variable.
> Yes and no. I am indeed trying to find where the //first// assignment occurred, since re-assigning to the pointer obtained from `get()` doesn't give any information regarding regarding the smart pointer being null or not. So what this visitor does that it goes back to the original assignment to find out what SVal was bound to `a.get()`. The Environment doesn't have this info since it is garbage collected somewhere on the way. Also accessing this State allows me to check whether the SVal was null to begin with.
> I don't think `FindLastStoreBRVisitor` does this.
I think it's too late to act when `.get()` is already happening. Like, we're visiting from bottom to top, in reverse to how our normal abstract interpretation goes. So if we only start tracking when we reach `.get()` we won't be able to explain what happens to the pointer between obtaining it from `.get()` and constraining it to null. For instance, if it was moved from one raw pointer variable to another, we won't put a note there, but we should.

Let's try the following. Write a visitor to detect the moment of time when the raw pointer value is getting constrained to null. I.e., find when `State->assume(State->get<TrackedRegionMap>(ThisRegion))` stops working. It can be for two reasons: either the inner value is overwritten or it's constrained to null. If it's overwritten, track the newly set value and our job is done. If it's constrained to null, try to find out what's happening (is it an if-statement? is it an eagerly-assume action over a comparison operator?). We already have a common visitor that's good at figuring this out, maybe it'd be possible to reuse the code. In any case, start tracking the symbol and possibly emit a checker-specific note immediately ("raw pointer value constrained to null" or something like that).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183