NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, baloghadamsoftware.
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 (?)
Repository:
rC Clang
https://reviews.llvm.org/D53543
Files:
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/NewDelete-custom.cpp
test/Analysis/NewDelete-sized-deallocation.cpp
Index: test/Analysis/NewDelete-sized-deallocation.cpp
===================================================================
--- /dev/null
+++ test/Analysis/NewDelete-sized-deallocation.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify %s
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify -fsized-deallocation %s
+
+void foo() {
+ int *x = new int;
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+
+void bar() {
+ int *x = new int;
+ delete x;
+} // no-warning
Index: test/Analysis/NewDelete-custom.cpp
===================================================================
--- test/Analysis/NewDelete-custom.cpp
+++ test/Analysis/NewDelete-custom.cpp
@@ -4,9 +4,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
#include "Inputs/system-header-simulator-cxx.h"
-#if !(LEAKS && !ALLOCATOR_INLINING)
// expected-no-diagnostics
-#endif
void *allocator(std::size_t size);
@@ -24,50 +22,37 @@
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() {
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -164,6 +164,7 @@
check::EndFunction,
check::PreCall,
check::PostStmt<CallExpr>,
+ check::PreStmt<CXXNewExpr>,
check::PostStmt<CXXNewExpr>,
check::NewAllocator,
check::PreStmt<CXXDeleteExpr>,
@@ -211,6 +212,7 @@
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkPreStmt(const CXXNewExpr *DE, CheckerContext &C) const;
void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
CheckerContext &C) const;
@@ -712,10 +714,8 @@
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)
@@ -726,23 +726,9 @@
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 iff it's not defined in a user file.
+ SourceLocation L = FD->getLocation();
+ return !L.isValid() || C.getSourceManager().isInSystemHeader(L);
}
llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc(
@@ -1084,15 +1070,17 @@
return false;
}
+void MallocChecker::checkPreStmt(const CXXNewExpr *NE,
+ CheckerContext &C) const {
+ 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);
+}
+
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;
@@ -2438,10 +2426,6 @@
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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
