core.StackAddressEscape checker produce invalid detection

[earnol]

The core.StackAddressEscape produce invalid disgnostics (false positive) in the following case:

#include <stdint.h>

class LocalClass;
class VEI {
public:
  VEI():pLocal(nullptr) {}  
  LocalClass *pLocal;
};

class LocalClass {
public:
    LocalClass(VEI *v):p(v) {}
    VEI *p;
    ~LocalClass() { p->pLocal = nullptr; }
    void updateVei() {
        if(p->pLocal != this) {
          p->pLocal = this;
        }
    }
};

intptr_t funct(VEI *vei) {
  LocalClass ohRly(vei);
  ohRly.updateVei();
  return (intptr_t)vei;
}

int main(void) {
    VEI vei;
    intptr_t res = funct(&vei);
    return res > 0x7FFF;
}

The problem is the program context is analyzed at the time of return (line 32) but actual removal of local variable reference happens in the destructor and then context is actually destroyed.
The godbolt reference demonstrating the same is available though this link.

Observed behavior:
The error 32:3: warning: Address of stack memory associated with local variable 'ohRly' returned to caller [core.StackAddressEscape] is reported.

Expected behavior:
No diagnostics reported as no actual stack escape happens here as vei.pLocal is actually equals to nullptr in line 38 if the provided example.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: None (earnol)

The core.StackAddressEscape produce invalid disgnostics (false positive) in the following case: ``` #include <stdint.h>

class LocalClass;
class VEI {
public:
VEI():pLocal(nullptr) {}
LocalClass *pLocal;
};

class LocalClass {
public:
LocalClass(VEI *v):p(v) {}
VEI *p;
~LocalClass() { p->pLocal = nullptr; }
void updateVei() {
if(p->pLocal != this) {
p->pLocal = this;
}
}
};

intptr_t funct(VEI *vei) {
LocalClass ohRly(vei);
ohRly.updateVei();
return (intptr_t)vei;
}

int main(void) {
VEI vei;
intptr_t res = funct(&vei);
return res > 0x7FFF;
}

The problem is the program context is analyzed at the time of return (line 32) but actual removal of local variable reference happens in the destructor and then context is actually destroyed.
The godbolt reference demonstrating the same is available though this [link](https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIAJykrgAyeAyYAHI%2BAEaYxCAAzGakAA6oCoRODB7evgGp6ZkCoeFRLLHxSbaY9o4CQgRMxAQ5Pn6BdpgOWQ1NBCWRMXGJyQqNza15HeP9YYPlw0kAlLaoXsTI7BwA9NsAtAeHR8cnp2fnF5dX1ze3dwcmGgCCuwDUT%2BjoxJgKCq%2BoVFeYyYyAA1q82CwSABPV5MX6oZB4JgETDoV4Ad0ICFetERYleADcmsjovRXmAOKgEAAlWjQymvb4EdbhdFEV6iWj0YivEwAVgsaG%2BADpemCPl8fgpsApRClMAKACKPF7bV4AFQQeD%2BWO5r1icNe6EMwFoYWATMwVDijE2fMFwswYsaEs%2B31%2Bsvliv5Kueu1VbwOhjE0IAXphXnsABoh%2BkR3l7OPhuJ7ZAILqguImBJKp2B/b3IvFkulssl1WVswJMLIbxYPkJNxjfCCEUIHPYSvPOvwv7BfG0NwGX45qw9kd/ABq2AAknyAOzjp4pLxkvDIECq16vGeziBLEApAdciAMLzclIEYhLRdWBdKnfb14nsTDvuvABUx8HY8rD7/Z5VV7X4X0Hd9QJMJdVVXddN2fHdXyHScID3L9CUPFIIAwu8oL9J4dx3NDv0AgjCIAPyQiCFAPO9XhSPZOx/LlG0fc9L2vMdF3wwiiVQPB0S8FITVRKdMDwWioOXXjeLwKgIAYpikIpMAc0fAhtQUW8pIQmSd0UhJsGYgk1NeDSdVIvSnwA55eLw/8VQSZdVTCAgr2IAB9AhXioLwGAcVC53Q8TtOg2ywK5aj/lpelsJCyzorpaERSEkTMDEiSlgS5lWVeCBXPcrylkJcTSPsoDnlciEmDCbD%2BPQULpMItCSrwBKdwK68vKtP5TN8/yCAgcwADZWqypzdJy4gGB6xtsFeDRVAXAAxVaypsp4OBWWhOH5Xg/A4LRSFQTg3GsawgTWDZI3MBIeFIAhNC2lZQRAflJBFfx%2BS4fxJAADg0MwpD%2B6t%2BX0ThJH2p7js4XgFBADQHqelY4FgJA1jcrwCHISg0BYFI6DiCJWC2VQ/uGvZhskTkDCMV4EhFamID7OI6gYLTeDRQgSAEvR%2BEEEQxHYKQZEERQVHUQ6dD0DFiCYFJOB4bbdqhqWYY4AB5LHV28gFXjJimqZp008uvPzQVvCAPHxwneVurgll4R6pZRpA8YJnkcYgd3bZQWngC84hzb4OhUWIeGIGiRXeGiMImmhaPSFj5hiGhDXom0LpnfuvG2EEDWGHpROsGiLxgDcMRaHh7heCwFhTXEGvSHwb5uhK6v7swVQuixrYjtcmpofNaI5dTjwsET688BYROSuIaJ0kwJVMHroxzSMZG%2BAMYAFCnPBMAxDWFQO%2B7%2BeELlhekM/xbUaHdC4fRTRQc7LH0PBonhyAVlQK8smr4NmDxkjDGZMCYoygNTOmTM2ZcxOijAAdQYKgPYfkvBKHQGmVALBV4YPXpgJMxBgA%2BEYAQXgqA57EAEj8eAKxOjdGcBAVwkw/APxCHMMoFQ9BpAyGzZhXDChswGBw4YD86Fs16BMTwbQ9BiJ6DMIRQx4iiJmHw5RfQFELCUbQq6mwJDKw4HtUgB0jonQ4PrcmlNqa9jphAM2DALZ5VwNzO21YHZO2RisDMTAsDxAPKQV6kh%2BQig0P9DQ31/ALj%2Bt9BcVNwYcEhqQGeXANCI2MWQ2GtgEZIxdvoswqsTEZOdloJYKw54ZGcJIIAA).

**Observed behavior**:
The error `32:3: warning: Address of stack memory associated with local variable 'ohRly' returned to caller [core.StackAddressEscape]` is reported.

**Expected behavior**:
No diagnostics reported as no actual stack escape happens here.



</details>

[steakhal]

Thanks for reporting this.
This issue can be reduced into this:
https://godbolt.org/z/Te8Ph8bvd

// --analyze -Xanalyzer -analyzer-checker=core,debug.ExprInspection,debug.DumpCFG
template <class T>
struct SetToNullInDtor {
  SetToNullInDtor(T **q) : q(q) {}
  ~SetToNullInDtor() { *q = nullptr; }
  T **q;
};

int **top(int **pp) {
  SetToNullInDtor guard(pp);
  int local;
  *pp = &local;
  return pp;
}

This issue is that we model the return pp statement before the destructors. Consequently, the side-effects of the dtors wouldn't be captured by the copy of p in the return value.

See the control-flow graph of this sample code. Notice that the dtor is after the return:

int **top(int **pp)
 [B2 (ENTRY)]
   Succs (1): B1

 [B1]
   1: pp
   2: [B1.1] (ImplicitCastExpr, LValueToRValue, int **)
   3: [B1.2] (CXXConstructExpr, [B1.4], SetToNullInDtor<int>)
   4: SetToNullInDtor guard(pp);
   5: int local;
   6: local
   7: &[B1.6]
   8: pp
   9: [B1.8] (ImplicitCastExpr, LValueToRValue, int **)
  10: *[B1.9]
  11: [B1.10] = [B1.7]
  12: pp
  13: [B1.12] (ImplicitCastExpr, LValueToRValue, int **)
  14: return [B1.13];  // <----- return
  15: [B1.4].~SetToNullInDtor<int>() (Implicit destructor)  // <------ dtor
   Preds (1): B2
   Succs (1): B0

Swapping the order would be easy, but doing so we would ruin the semantics of std::unique_ptr which would release the memory before we could return it :D
So, fixing this needs some careful thinking.

[earnol]

Yes. I debugged this issue a little and the reason is the checker uses check:PreStmt<ReturnStmt> which obviously happens before implicit destructors are called.
One of the possible solutions could be to enumerate all implicit destructors after return statement and call the check on the state after those are finished, but it sounds a bit overcomplicated and also might cause issues i currently does not see.

[steakhal]

If you are there, could you check if check::EndFunction would be invoked after the dtors? I suspect it should.
If that would be the case, we should just migrate the check to that callback to fix this.

[earnol]

It should be true for clangSA as destructor function is still a function, but let me test it.
Also I think it is important to note, that we can have multiple destructors and we need to find the "last one" correctly. Do not have a good ideas about it.

[steakhal]

It should be true for clangSA as destructor function is still a function, but let me test it. Also I think it is important to note, that we can have multiple destructors and we need to find the "last one" correctly. Do not have a good ideas about it.

My idea is to not check the return statements of functions but the end of functions.
In this case it would mean that when the function "top" "ends" (thus invokes the check::EndFunction callback), the dtors of the local variables within top should be already done.

[earnol]

Yes it should work theoretically, but there is a detection problem. Let me explain.
I made a change:

diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 019e81f91400..7aa13c715626 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -450,6 +450,15 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
   if (!StackAddrEscape.isEnabled())
     return;

+  const LocationContext *LC = Ctx.getLocationContext();
+  const Decl *D = LC->getDecl();
+  const FunctionDecl *FD = D->getAsFunction();
+
+  if (FD) {
+    DeclarationNameInfo NameInfo = FD->getNameInfo();
+    // Output function name
+    llvm::errs() << "Finished analyzing function: " <<         NameInfo.getAsString() << "\n";
+  }
   ExplodedNode *Node = Ctx.getPredecessor();

   bool ExitingTopFrame =

and got diagnostics:

Finished analyzing function: VEI
Finished analyzing function: LocalClass
Finished analyzing function: updateVei
Finished analyzing function: ~LocalClass
Finished analyzing function: funct
Finished analyzing function: main

For the example i provided, so as soon as we are able to catch the latest destructor, we are fine, but how we are going to deduce which one is latest?
I have only weird idea with using coroutines and captured context, but it did to worked for me because while i do capture context the checker context is not actually captured.

[earnol]

And if i'm to use slightly modified sample:

class LocalClass;
class VEI {
public:
  VEI():pLocal(nullptr) {}  
  LocalClass *pLocal;
};

class LocalClass {
public:
    LocalClass(VEI *v):p(v) {}
    VEI *p;
    ~LocalClass() { p->pLocal = nullptr; }
    void updateVei() {
        if(p->pLocal != this) {
          p->pLocal = this;
        }
    }
};

long funct(VEI *vei) {
  LocalClass ohRly(vei);
  LocalClass oRly(vei);
  ohRly.updateVei();
  oRly.updateVei();
  if((long)vei > 0x9FFF)
  {
    return (long)vei;
  }
  return 0;
}

int main(void) {
    VEI vei;
    long res = funct(&vei);
    return res > 0x7FFF;
}

The diagnostics become:

Finished analyzing function: VEI
Finished analyzing function: LocalClass
Finished analyzing function: LocalClass
Finished analyzing function: updateVei
Finished analyzing function: updateVei
Finished analyzing function: ~LocalClass
Finished analyzing function: ~LocalClass
Finished analyzing function: funct
Finished analyzing function: main
Finished analyzing function: ~LocalClass
Finished analyzing function: ~LocalClass
Finished analyzing function: funct
Finished analyzing function: main

Making it even more complex. My current idea which might work as to keep PreStmt callback and same a location of latest return statement. Then run actual check on function exist from the actual context of the ReturnStmt. And use saved location in the diagnostics.
This way the checker will work correctly albeit having a little convoluted implementation. It should also get rid of false positive.