[analyzer][Review request] Partial fix for PR19102.

[ayartsev]

Summary

Attached patch is a partial fix for PR19102. The idea is to assume the unused operator new as not a leak if the following requirement is met:

• The invoked constructor has a parameter - pointer or reference to a record that has a public method (any access level , if a constructed record is a friend of a parameter record), that accepts a pointer to the type of the constructed record or one of its bases.

The analysis can be much more precise if the analyzer can analyze the body of the constructor, but for now the criteria above can be used regardless of whether the body of the constructor is accessible to the analyzer or not.
Please review.

[jordan_rose]

This is searching all the methods on the class being passed, but what about its superclasses?

You should definitely be caching this information. I'm also just still worried that this is a very expensive search for every record parameter of every constructor, even if you collected all the types referenced by each class instead of just looking for a particular one.

[ayartsev]

Now searching on the class and all superclasses that has methods accessible from the constructed record. Referenced types are now cached.

The search is expensive, but the case is very rare I hope. Only unassigned new are analyzed this way.

[jordan_rose]

Now that I'm paging this back in...I'm not convinced. People who use unassigned new usually know what they're doing. The false positives come when someone uses new while storing the result, but they also escape the variable into storage somewhere.

For the unused result case, I wonder if a -Wunused-like warning would cover this: warn on un-consumed new, silence it by casting to void, and assume any un-consumed new has already escaped (unless we inlined the constructor or can see it is trivial).

I'd be tempted to ignore the used case except that that's blocking adoption in LLVM. We'd have to get sign-on from llvmdev if we wanted to change the new Value(module, ...) idiom they have going on there (e.g. to something like Value::create(module, ...).)

[ayartsev]

Simplified the check, an un-consumed new is now treated a leak unless the constructor has a parameter of a pointer-to-record type. The check is not expensive and covers LLVM cases.

Did not added -Wunused-like new warning for now. Do you think this warning is needed apart from the checker? Doubt it ever be used. Anyway I think it is better to implement the warning as a separate patch as the change is not analyzer specific.

Is there a way to get the constructor inlined when it is called from a new expression? ExprEngine::VisitCXXConstructExpr() always left 'wasInlined' equal 'false'.

[ayartsev]

Ping!

[jordan_rose]

IIRC, constructors are never inlined for 'new' right now because we never got the right region, and I was always hoping we'd implement proper allocator support first (http://llvm.org/bugs/show_bug.cgi?id=12014). Karthik Bhat was working on this but I don't remember how far he got. We also didn't have destructor support for 'delete' until last fall (also thanks to Karthik).

Other than that this is looking pretty good. More tests would be nice, and possibly a test on a real code base that would otherwise have false positives. Unfortunately the only one I can think of is LLVM itself, but even that could tell us how many false positives we've gone down by.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

768

Style nitpick: * next to the variable name.

772–773

getAsCXXRecordDecl will call getCanonicalType for you, so you can skip that step.

776

Please make this const (and move the *).

785–786

I don't think you need to worry about either canonicalizing the type or stripping qualifiers.

[ayartsev]

Done! Ok to commit?
Just ran the updated NewDeleteLeaks checker over LLVM codebase with -analyzer-opt-analyze-headers option turned on. Failed to attach reports in Phabricator, I'll send them with the next mail.

[ayartsev]

Attached are leak reports generated by NewDeleteLeaks checker ran over
the LLVM codebase with -analyzer-opt-analyze-headers option turned on.
Haven't addressed them yet. Looking at suspicious leak reports generated
from headers (e.g. reports from TinyPtrVector.h).

• NewDeleteLeaks_reports.zip423 KBDownload

[jordan_rose]

You're still missing tests that show that the leaks are reported no matter what the arguments are if the newly-created instance is consumed. But other than that this looks good; please commit once you've added those!

(Yes, I know they're present elsewhere in the test suite, but it's good to have some explicit ones here.)

[ayartsev]

Committed as r214909.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 // This file defines malloc/free checker, which checks for potential memory
 // leaks, double free, and use-after-free problems.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "InterCheckerAPI.h"
 #include "clang/AST/Attr.h"
+#include "clang/AST/ParentMap.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
if (FD->hasAttrs())
           State = FreeMemAttr(C, CE, I);
           break;
         }
       }
   }
   C.addTransition(State);
 }
 
+static QualType getDeepPointeeType(QualType T) {
+  QualType Result = T, PointeeType = T->getPointeeType();
+  while (!PointeeType.isNull()) {
+    Result = PointeeType;
+    PointeeType = PointeeType->getPointeeType();
+  }
+  return Result;
+}
+
+static bool treatUnusedNewEscaped(const CXXNewExpr *NE) {
+
+  const CXXConstructExpr *ConstructE = NE->getConstructExpr();
+  if (!ConstructE)
+    return false;
+
+  if (!NE->getAllocatedType()->getAsCXXRecordDecl())
+    return false;
+
+  const CXXConstructorDecl *CtorD = ConstructE->getConstructor();
+
+  // Iterate over the constructor parameters.
+  for (const auto *CtorParam : CtorD->params()) {
+
+    QualType CtorParamPointeeT = CtorParam->getType()->getPointeeType();
+    if (CtorParamPointeeT.isNull())
+      continue;
+
+    CtorParamPointeeT = getDeepPointeeType(CtorParamPointeeT);
+
+    if (CtorParamPointeeT->getAsCXXRecordDecl())
+      return true;
+  }
+
+  return false;
+}
+
 void MallocChecker::checkPostStmt(const CXXNewExpr *NE, 
                                   CheckerContext &C) const {
 
   if (NE->getNumPlacementArgs())
     for (CXXNewExpr::const_arg_iterator I = NE->placement_arg_begin(),
          E = NE->placement_arg_end(); I != E; ++I)
       if (SymbolRef Sym = C.getSVal(*I).getAsSymbol())
         checkUseAfterFree(Sym, C, *I);
 
   if (!isStandardNewDelete(NE->getOperatorNew(), C.getASTContext()))
     return;
 
+  ParentMap &PM = C.getLocationContext()->getParentMap();
+  if (!PM.isConsumedExpr(NE) && treatUnusedNewEscaped(NE))
+    return;
+
   ProgramStateRef State = C.getState();
   // The return value from operator new is bound to a specified initialization 
   // value (if any) and we don't want to loose this value. So we call 
   // MallocUpdateRefState() instead of MallocMemAux() which breakes the 
   // existing binding.
   State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray 
                                                            : AF_CXXNew);
   C.addTransition(State);

test/Analysis/NewDeleteLeaks-PR19102.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.cplusplus.NewDeleteLeaks -verify %s
+
+class A0 {};
+
+class A1 {
+public:
+  A1(int);
+};
+
+struct S{
+  int i;
+};
+
+class A2 {
+public:
+  A2();
+  A2(S);
+  A2(int*);
+  A2(S*);
+  A2(S&, int);
+  A2(int, S**);
+};
+
+void test() {
+  new int; // expected-warning@+1 {{Potential memory leak}}
+  new A0; // expected-warning@+1 {{Potential memory leak}}
+  new A1(0); // expected-warning@+1 {{Potential memory leak}}
+  new A2; // expected-warning@+1 {{Potential memory leak}}
+  S s;
+  s.i = 1;
+  S* ps = new S;
+  new A2(s); // expected-warning@+1 {{Potential memory leak}}
+  new A2(&(s.i)); // expected-warning@+1 {{Potential memory leak}}
+  new A2(ps); // no warning
+  new A2(*ps, 1); // no warning
+  new A2(1, &ps); // no warning
+}