[analyzer][MallocChecker] Improve warning messages on double-delete errors

[NoQ]

Thanks, nice catch!

It seems that the ReportDoubleDelete() thing was never used for reporting double-delete, but it was used for some strange check when a destructor is called. Is that old code even correct?

[Szelethus]

In D54834#1315475, @NoQ wrote:

Thanks, nice catch!

It seems that the ReportDoubleDelete() thing was never used for reporting double-delete, but it was used for some strange check when a destructor is called.

It was used for reporting, there actually is a test case for it in test/Analysis/NewDelete-checker-test.cpp on line 380.

[Szelethus]

test/Analysis/NewDelete-checker-test.cpp

380 ↗

(On Diff #175054)

Right here.

392 ↗

(On Diff #175054)

And here.

[NoQ]

Mm, yeah, but it's still weird to see it in checkPreCall for CXXDestructorCall.

I suspect that your new warning covers these tests as well.

[NoQ]

Ugh. MSVC buildbot is acting weird again. Also it is not acting deterministically.

I was also yelled at by it in D55388.

Which makes me suspect that it has something to do with checker registration, as i change checker option related behavior in my patch, and the crashes in MallocChecker deal with loading checker name.

[NoQ]

Wait, no, i didn't do anything with checker options yet. Something else then, i guess.

[NoQ]

Ok, D55388 seems to have landed (3 builds without failures in move checker), so i guess these are separate problems after all.

The first failing build seems to be http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/2662 - it includes both this commit and rC349281.

[Szelethus]

Interesting, I've been watching the bots closely, but got no mail after a while. I'm not sure what the cause is, so I'll revert one-by-one.

[Szelethus]

Reverted in rC349340. With the wrong revision name... oops...

[NoQ]

In D54834#1332679, @Szelethus wrote:

Interesting, I've been watching the bots closely, but got no mail after a while. I'm not sure what the cause is, so I'll revert one-by-one.

One of the common reasons for that is that the buildbot was already failing when you committed your stuff. Which is why for many buildbots there are special people who look at them and tell people to fix their stuff manually.

This time it wasn't the case though: build 2661 was green. So dunno. I definitely did receive a mail from this buildbot when it failed on my patch.

But generally it's fine if you don't notice that you're breaking a buildbot or two. It's not something to worry about. Sooner or later, they will come after you :)

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
         State, Extent, SizeInBytes.castAs<DefinedOrUnknownSVal>());
     State = State->assume(extentMatchesSize, true);
   }
   return State;
 }
 
 void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE,
                                  CheckerContext &C) const {
-
+  // This will regard deleting freed() regions as a use-after-free, rather then
+  // a double-free or double-delete error.
   if (!ChecksEnabled[CK_NewDeleteChecker])
     if (SymbolRef Sym = C.getSVal(DE->getArgument()).getAsSymbol())
       checkUseAfterFree(Sym, C, DE->getArgument());
 
   if (!MemFunctionInfo.isStandardNewDelete(DE->getOperatorDelete(),
                                            C.getASTContext()))
     return;
 
ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
   if (CE->getNumArgs() < (Num + 1))
     return nullptr;
 
   return FreeMemAux(C, CE->getArg(Num), CE, State, Hold,
                     IsKnownToBeAllocated, ReturnsNullOnFailure);
 }
 
 /// Checks if the previous call to free on the given symbol failed - if free
-/// failed, returns true. Also, returns the corresponding return value symbol.
+/// failed, returns true. Also, stores the corresponding return value symbol in
+/// \p RetStatusSymbol.
 static bool didPreviousFreeFail(ProgramStateRef State,
                                 SymbolRef Sym, SymbolRef &RetStatusSymbol) {
   const SymbolRef *Ret = State->get<FreeReturnValue>(Sym);
   if (Ret) {
     assert(*Ret && "We should not store the null return symbol");
     ConstraintManager &CMgr = State->getConstraintManager();
     ConditionTruthVal FreeFailed = CMgr.isNull(State, *Ret);
     RetStatusSymbol = *Ret;
void MallocChecker::ReportDoubleFree(CheckerContext &C, SourceRange Range,
   if (!ChecksEnabled[CK_MallocChecker] &&
       !ChecksEnabled[CK_NewDeleteChecker])
     return;
 
   Optional<MallocChecker::CheckKind> CheckKind = getCheckIfTracked(C, Sym);
   if (!CheckKind.hasValue())
     return;
 
+  // If this is a double delete error, print the appropiate warning message.
+  if (CheckKind == CK_NewDeleteChecker) {
+    ReportDoubleDelete(C, Sym);
+    return;
+  }
+
   if (ExplodedNode *N = C.generateErrorNode()) {
     if (!BT_DoubleFree[*CheckKind])
       BT_DoubleFree[*CheckKind].reset(new BugType(
           CheckNames[*CheckKind], "Double free", categories::MemoryError));
 
     auto R = llvm::make_unique<BugReport>(
         *BT_DoubleFree[*CheckKind],
         (Released ? "Attempt to free released memory"

clang/test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist

<array>
          <key>line</key><integer>14</integer>
          <key>col</key><integer>10</integer>
          <key>file</key><integer>0</integer>
         </dict>
        </array>
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Attempt to free released memory</string>
+     <string>Attempt to delete released memory</string>
      <key>message</key>
-     <string>Attempt to free released memory</string>
+     <string>Attempt to delete released memory</string>
     </dict>
    </array>
-   <key>description</key><string>Attempt to free released memory</string>
+   <key>description</key><string>Attempt to delete released memory</string>
    <key>category</key><string>Memory error</string>
-   <key>type</key><string>Double free</string>
+   <key>type</key><string>Double delete</string>
    <key>check_name</key><string>cplusplus.NewDelete</string>
    <!-- This hash is experimental and going to change! -->
-   <key>issue_hash_content_of_line_in_context</key><string>bd8e324d09c70b9e2be6f824a4942e5a</string>
+   <key>issue_hash_content_of_line_in_context</key><string>593b185245106bed5175ccf2753039b2</string>
   <key>issue_context_kind</key><string>function</string>
   <key>issue_context</key><string>test</string>
   <key>issue_hash_function_offset</key><string>8</string>
   <key>location</key>
   <dict>
    <key>line</key><integer>14</integer>
    <key>col</key><integer>3</integer>
    <key>file</key><integer>0</integer>
<array>
          <key>line</key><integer>27</integer>
          <key>col</key><integer>11</integer>
          <key>file</key><integer>0</integer>
         </dict>
        </array>
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Attempt to free released memory</string>
+     <string>Attempt to delete released memory</string>
      <key>message</key>
-     <string>Attempt to free released memory</string>
+     <string>Attempt to delete released memory</string>
     </dict>
    </array>
-   <key>description</key><string>Attempt to free released memory</string>
+   <key>description</key><string>Attempt to delete released memory</string>
    <key>category</key><string>Memory error</string>
-   <key>type</key><string>Double free</string>
+   <key>type</key><string>Double delete</string>
    <key>check_name</key><string>cplusplus.NewDelete</string>
    <!-- This hash is experimental and going to change! -->
-   <key>issue_hash_content_of_line_in_context</key><string>8bf1a5b9fdae9d86780aa6c4cdce2605</string>
+   <key>issue_hash_content_of_line_in_context</key><string>6484e9b006ede7362edef2187ba6eb37</string>
   <key>issue_context_kind</key><string>function</string>
   <key>issue_context</key><string>test</string>
   <key>issue_hash_function_offset</key><string>3</string>
   <key>location</key>
   <dict>
    <key>line</key><integer>27</integer>
    <key>col</key><integer>2</integer>
    <key>file</key><integer>0</integer>

clang/test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp

 }
 
 //----------------------------------------------------------------
 // Check that alpha.cplusplus.NewDelete catches all types of bugs.
 //----------------------------------------------------------------
 void testNewDoubleFree() {
   int *p = new int;
   delete p;
-  delete p; // expected-warning{{Attempt to free released memory}}
+  delete p; // expected-warning{{Attempt to delete released memory}}
 }
 
 void testNewLeak() {
   int *p = new int;
 }
 #ifdef LEAKS
 // expected-warning@-2 {{Potential leak of memory pointed to by 'p'}}
 #endif

clang/test/Analysis/NewDelete-checker-test.cpp

void testUseThisAfterDelete() {
   SomeClass *c = new SomeClass;
   delete c;
   c->f(0); // expected-warning{{Use of memory after it is freed}}
 }
 
 void testDoubleDelete() {
   int *p = new int;
   delete p;
-  delete p; // expected-warning{{Attempt to free released memory}}
+  delete p; // expected-warning{{Attempt to delete released memory}}
 }
 
 void testExprDeleteArg() {
   int i;
   delete &i; // expected-warning{{Argument to 'delete' is the address of the local variable 'i', which is not memory allocated by 'new'}}
 }
 
 void testExprDeleteArrArg() {

clang/test/Analysis/NewDelete-path-notes.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.NewDelete,unix.Malloc -analyzer-output=text -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.NewDelete,unix.Malloc -analyzer-output=text -analyzer-config c++-allocator-inlining=true -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.NewDelete,unix.Malloc -analyzer-output=plist %s -o %t.plist
 // RUN: cat %t.plist | %diff_plist %S/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
 
 void test() {
   int *p = new int;
   // expected-note@-1 {{Memory is allocated}}
   if (p)
     // expected-note@-1 {{Taking true branch}}
     delete p;
     // expected-note@-1 {{Memory is released}}
 
-  delete p; // expected-warning {{Attempt to free released memory}}
-  // expected-note@-1 {{Attempt to free released memory}}
+  delete p; // expected-warning {{Attempt to delete released memory}}
+  // expected-note@-1 {{Attempt to delete released memory}}
 }
 
 struct Odd {
   void kill() {
     delete this; // expected-note {{Memory is released}}
   }
 };
 
 void test(Odd *odd) {
   odd->kill(); // expected-note{{Calling 'Odd::kill'}}
                // expected-note@-1 {{Returning; memory was released}}
-  delete odd; // expected-warning {{Attempt to free released memory}}
-              // expected-note@-1 {{Attempt to free released memory}}
+  delete odd; // expected-warning {{Attempt to delete released memory}}
+              // expected-note@-1 {{Attempt to delete released memory}}
 }
 

clang/test/Analysis/dtor.cpp

NonTrivial(const NonTrivial &other) {
     p = new int[1];
     do_copy(p, other.p, 1);
   }
   NonTrivial &operator=(const NonTrivial &other) {
     p = other.p;
     return *this;
   }
   ~NonTrivial() {
-    delete[] p; // expected-warning {{free released memory}}
+    delete[] p; // expected-warning {{delete released memory}}
   }
 };
 
 void f() {
   NonTrivial nt1;
   NonTrivial nt2(nt1);
   nt1 = nt2;
   clang_analyzer_eval(__is_trivial(NonTrivial)); // expected-warning{{FALSE}}
   clang_analyzer_eval(__alignof(NonTrivial) > 0); // expected-warning{{TRUE}}
 }
 }