[analyzer] MallocChecker: pr39348: Realize that sized delete isn't a custom delete.

[NoQ]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=39348

Under -fsized-deallocation, the new test case in NewDelete-sized-deallocation.cpp produces a call to operator delete with an explicitly passed size argument. MallocChecker didn't expect that and identified this as a custom operator delete, which lead to a leak false positive on extremely simple code.

Identify custom operators by source locations instead of guessing by the number of parameters. This sounds much more correct.

Additionally, it exposes a regression in NewDelete-intersections.mm's testStandardPlacementNewAfterDelete() test, where the diagnostic is delayed from before the call of placement new into the code of placement new in the header. This happens because the check for pass-into-function-after-free for placement arguments is located in checkNewAllocator(), which happens after the allocator is inlined, which is too late. Move this use-after-free check into checkPreCall instead, where it works automagically because the guard that prevents it from working is useless and can be removed as well.

I didn't bother looking at NewDelete-custom.cpp because they only manifest under the non-default-and-never-planned-to-be-default-anymore option -analyzer-config c++-allocator-inlining=false. I believe that this flag can be removed entirely. I think we've previously been thinking that these are false negatives in the new mode that are potentially undesired. Did anybody actually end up using it? If a more aggressive behavior towards custom allocators is desired, would it make sense to re-implement it directly, without relying on side effects of broken operator new modeling (?)

[NoQ]

Nono, that was an old patch, sry. Here's the correct patch.

[NoQ]

Remove weird flags that i used for debugging from the test run-lines.

[MTC]

test/Analysis/NewDelete-custom.cpp

7 ↗

(On Diff #170543)

Should we continue to keep this line?

test/Analysis/NewDelete-sized-deallocation.cpp

1 ↗

(On Diff #170543)

I believe sized deallocation is the feature since C++14, see https://en.cppreference.com/w/cpp/memory/new/operator_delete.

[george.karpenkov]

because the guard that prevents it from working is useless and can be removed as well

Should we remove it then?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

729 ↗

(On Diff #170543)

Should we return true on invalid source location?
Do we have a test case for that?

2444 ↗

(On Diff #170543)

why it's fine to remove this branch?

test/Analysis/NewDelete-custom.cpp

7 ↗

(On Diff #170543)

yes, since there are not diagnostics emitted. it would complain otherwise.

test/Analysis/NewDelete-sized-deallocation.cpp

1 ↗

(On Diff #170543)

specifying 17 is also fine

[NoQ]

Add tests for different versions of the standard and for different ways to declare operator delete.

Should we remove it then?

why it's fine to remove this branch?

Because these two questions refer to the same branch (:

[MTC]

test/Analysis/NewDelete-sized-deallocation.cpp

1 ↗

(On Diff #170543)

Yea, that's true.

[Szelethus]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

727 ↗

(On Diff #170704)

typo: if

test/Analysis/NewDelete-custom.cpp

3–4 ↗

(On Diff #170704)

Do we still need -DLEAKS=1 and -DALLOCATOR_INLINING=1?

[NoQ]

Address comments!

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

727 ↗

(On Diff #170704)

https://en.wikipedia.org/wiki/If_and_only_if

Thx for pointing out that it's not an entirely common jargon!

test/Analysis/NewDelete-custom.cpp

3–4 ↗

(On Diff #170704)

I guess i'll just remove the no-leaks run-lines because they didn't make sense in the first place.

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

for (const auto *I : FD->specific_attrs<OwnershipAttr>()) {
           return true;
       }
     }
   }
 
   return false;
 }
 
-// Tells if the callee is one of the following:
-// 1) A global non-placement new/delete operator function.
-// 2) A global placement operator function with the single placement argument
-//    of type std::nothrow_t.
+// Tells if the callee is one of the builtin new/delete operators, including
+// placement operators and other standard overloads.
 bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
                                         ASTContext &C) const {
   if (!FD)
     return false;
 
   OverloadedOperatorKind Kind = FD->getOverloadedOperator();
   if (Kind != OO_New && Kind != OO_Array_New &&
       Kind != OO_Delete && Kind != OO_Array_Delete)
     return false;
 
-  // Skip all operator new/delete methods.
-  if (isa<CXXMethodDecl>(FD))
-    return false;
-
-  // Return true if tested operator is a standard placement nothrow operator.
-  if (FD->getNumParams() == 2) {
-    QualType T = FD->getParamDecl(1)->getType();
-    if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-      return II->getName().equals("nothrow_t");
-  }
-
-  // Skip placement operators.
-  if (FD->getNumParams() != 1 || FD->isVariadic())
-    return false;
-
-  // One of the standard new/new[]/delete/delete[] non-placement operators.
-  return true;
+  // This is standard if and only if it's not defined in a user file.
+  SourceLocation L = FD->getLocation();
+  // If the header for operator delete is not included, it's still defined
+  // in an invalid source location. Check to make sure we don't crash.
+  return !L.isValid() || C.getSourceManager().isInSystemHeader(L);
 }
 
 llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc(
   const CallExpr *CE, CheckerContext &C, const ProgramStateRef &State) const {
   // 3-argument malloc(), as commonly used in {Free,Net,Open}BSD Kernels:
   //
   // void *malloc(unsigned long size, struct malloc_type *mtp, int flags);
   //
static bool treatUnusedNewEscaped(const CXXNewExpr *NE) {
   }
 
   return false;
 }
 
 void MallocChecker::processNewAllocation(const CXXNewExpr *NE,
                                          CheckerContext &C,
                                          SVal Target) 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();
if (!FD)
       return;
 
     ASTContext &Ctx = C.getASTContext();
     if (ChecksEnabled[CK_MallocChecker] &&
         (isCMemFunction(FD, Ctx, AF_Malloc, MemoryOperationKind::MOK_Free) ||
          isCMemFunction(FD, Ctx, AF_IfNameIndex,
                         MemoryOperationKind::MOK_Free)))
       return;
-
-    if (ChecksEnabled[CK_NewDeleteChecker] &&
-        isStandardNewDelete(FD, Ctx))
-      return;
   }
 
   // Check if the callee of a method is deleted.
   if (const CXXInstanceCall *CC = dyn_cast<CXXInstanceCall>(&Call)) {
     SymbolRef Sym = CC->getCXXThisVal().getAsSymbol();
     if (!Sym || checkUseAfterFree(Sym, C, CC->getCXXThisExpr()))
       return;
   }

cfe/trunk/test/Analysis/NewDelete-custom.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=false -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=false -DLEAKS=1 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -DALLOCATOR_INLINING=1 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -fblocks -verify %s -analyzer-config c++-allocator-inlining=false
 #include "Inputs/system-header-simulator-cxx.h"
 
-#if !(LEAKS && !ALLOCATOR_INLINING)
 // expected-no-diagnostics
-#endif
 
 
 void *allocator(std::size_t size);
 
 void *operator new[](std::size_t size) throw() { return allocator(size); }
 void *operator new(std::size_t size) throw() { return allocator(size); }
 void *operator new(std::size_t size, const std::nothrow_t &nothrow) throw() { return allocator(size); }
 void *operator new(std::size_t, double d);
 
 class C {
 public:
   void *operator new(std::size_t);  
 };
 
 void testNewMethod() {
   void *p1 = C::operator new(0); // no warn
 
-  C *p2 = new C; // no warn
+  C *p2 = new C; // no-warning
 
-  C *c3 = ::new C;
+  C *c3 = ::new C; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'c3'}}
-#endif
 
 void testOpNewArray() {
   void *p = operator new[](0); // call is inlined, no warn
 }
 
 void testNewExprArray() {
-  int *p = new int[0];
+  int *p = new int[0]; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
-#endif
 
 
 //----- Custom non-placement operators
 void testOpNew() {
   void *p = operator new(0); // call is inlined, no warn
 }
 
 void testNewExpr() {
-  int *p = new int;
+  int *p = new int; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
-#endif
-
 
 //----- Custom NoThrow placement operators
 void testOpNewNoThrow() {
   void *p = operator new(0, std::nothrow); // call is inlined, no warn
 }
 
 void testNewExprNoThrow() {
-  int *p = new(std::nothrow) int;
+  int *p = new(std::nothrow) int; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
-#endif
 
 //----- Custom placement operators
 void testOpNewPlacement() {
   void *p = operator new(0, 0.1); // no warn
 }
 
 void testNewExprPlacement() {
   int *p = new(0.1) int; // no warn
 }

cfe/trunk/test/Analysis/NewDelete-sized-deallocation.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS -fsized-deallocation
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS -fsized-deallocation
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS -fsized-deallocation
+
+// Test all three: undeclared operator delete, operator delete forward-declared
+// in the system header, operator delete defined in system header.
+#ifdef INCLUDE_INCLUDES
+// TEST_INLINABLE_ALLOCATORS is used within this include.
+#include "Inputs/system-header-simulator-cxx.h"
+#endif
+
+void leak() {
+  int *x = new int; // expected-note{{Memory is allocated}}
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+  // expected-note@-1{{Potential leak of memory pointed to by 'x'}}
+
+// This function was incorrectly diagnosed as leak under -fsized-deallocation
+// because the sized operator delete was mistaken for a custom delete.
+void no_leak() {
+  int *x = new int; // no-note
+  delete x;
+} // no-warning