[Analyzer] Correct stack address escape diagnostic

[FlameTop]

Summary

Leaking a stack address via a static variable refers to it in the diagnostic as a 'global'. This patch corrects the diagnostic for static variables.

Patch by Phil Camp, SN Systems

[zaks.anna]

Thanks for fixing!

Devin, what do you think about the BugType wording?

llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

229

I don't like the '/' here. The only idea I have is to replace it with "into a variable with static allocation", which is also not ideal because it uses jargon.

[dcoughlin]

llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

229

We have to be careful about changing the 'Name' parameter here. This is supposed to be stable, because we use it for issue hashing. Any change will cause the issue hash to change and stymie use of the issue hash for baselining/issue suppression.

I think we should probably not change it. It isn't user facing, so improving the text won't make a difference in user experience (although to does show up in plists as the "type" of the bug (via the "BugType" field in PathDiagnostic).

232

This string here is actually never used, so you should just remove it and use the constructor of BuiltinBug that doesn't take a description.

242

This is great!

[FlameTop]

Thank you for your review. I have removed the changes to the bug description. Apologies, I did not realize it would alter the hash.

[FlameTop]

Thank you for the acceptance.

May I please ask you to commit the change for me as I do not have commit access?

cheers

Phil Camp

[seaneveson]

Committed: http://reviews.llvm.org/rL270849

llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp

void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const {
 
   // Generate an error node.
   ExplodedNode *N = Ctx.generateNonFatalErrorNode(state);
   if (!N)
     return;
 
   if (!BT_stackleak)
     BT_stackleak.reset(
-        new BuiltinBug(this, "Stack address stored into global variable",
-                       "Stack address was saved into a global variable. "
+        new BuiltinBug(this, "Stack address stored into global/static variable",
+                       "Stack address was saved into a global/static variable. "
                        "This is dangerous because the address will become "
                        "invalid after returning from the function"));
 
   for (unsigned i = 0, e = cb.V.size(); i != e; ++i) {
     // Generate a report for this bug.
     SmallString<512> buf;
     llvm::raw_svector_ostream os(buf);
     SourceRange range = genName(os, cb.V[i].second, Ctx.getASTContext());
-    os << " is still referred to by the global variable '";
+    os << " is still referred to by the ";
+    if (isa<StaticGlobalSpaceRegion>(cb.V[i].first->getMemorySpace()))
+      os << "static";
+    else
+      os << "global";
+    os << " variable '";
     const VarRegion *VR = cast<VarRegion>(cb.V[i].first->getBaseRegion());
     os << *VR->getDecl()
        << "' upon returning to the caller.  This will be a dangling reference";
     auto report = llvm::make_unique<BugReport>(*BT_stackleak, os.str(), N);
     if (range.isValid())
       report->addRange(range);
 
     Ctx.emitReport(std::move(report));
   }
 }
 
 void ento::registerStackAddrEscapeChecker(CheckerManager &mgr) {
   mgr.registerChecker<StackAddrEscapeChecker>();
 }

llvm/tools/clang/test/Analysis/stackaddrleak.c

void f1() {
   p = str; 
   p = 0; // no-warning
 }
 
 void f2() {
   p = (const char *) __builtin_alloca(12);
 } // expected-warning{{Address of stack memory allocated by call to alloca() on line 19 is still referred to by the global variable 'p' upon returning to the caller.  This will be a dangling reference}}
 
-// PR 7383 - previosly the stack address checker would crash on this example
+// PR 7383 - previously the stack address checker would crash on this example
 //  because it would attempt to do a direct load from 'pr7383_list'. 
 static int pr7383(__const char *__)
 {
   return 0;
 }
 extern __const char *__const pr7383_list[];
 
 // Test that we catch multiple returns via globals when analyzing a function.
 void test_multi_return() {
   static int *a, *b;
   int x;
   a = &x;
   b = &x;
-} // expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the global variable 'a' upon returning}} expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the global variable 'b' upon returning}}
+} // expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the static variable 'a' upon returning}} expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the static variable 'b' upon returning}}
 
 intptr_t returnAsNonLoc() {
   int x;
   return (intptr_t)&x; // expected-warning{{Address of stack memory associated with local variable 'x' returned to caller}}
 }
 
 bool returnAsBool() {
   int x;