On 27.03.2013 5:23, Jordan Rose wrote:
On Mar 26, 2013, at 17:10 , Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
On 25.03.2013 21:39, Anna Zaks wrote:
On Mar 25, 2013, at 9:30 AM, Jordan Rose <[email protected]
<mailto:[email protected]>> wrote:
On Mar 25, 2013, at 8:01 , Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
Committed as r177849
Manage to find several random real bugs (report-843813.html,
report-230257.html, recursive case in report-727931.html), but for
now it is hard to detect real bugs among tons of false-positives.
There are two types of false-positives that form the majority of
reports:
1) Illustrated by the following test (added similar test to
NewDelete-checker-test.mm):
int *global;
void testMemIsOnHeap() {
int *p = new int; // FIXME: currently not heap allocated!
if (global != p) // evaluates to UnknownVal() rather then 'true'
global = p;
} // report false-positive leak
As I understand the problem is that currently a memory region for
'new' is not a heap region and this lead to false-positives like
report-863595.html and others. (e.g. that causes 'global != p'
evaluate to UnknownVal() rather then 'true' (logic of
SimpleSValBuilder::evalBinOpLL))
Attached is the proposed patch that fixes these issues.
There are two reasons I didn't use getConjuredHeapSymbol when I
originally put in this code:
(1) It handles all CXXNewExprs, even if the allocator is not one of
the global ones.
(2) Heap symbols weren't used yet (Anna added them later for
MallocChecker).
Obviously #2 is bogus now. #1 worries me a bit because it requires
duplicating some of the checks you just added to MallocChecker.
In the long run, the design would be to get the appropriate memory
from the allocator call, and we have PR12014's restructuring of the
CFG blocking that. I'm not sure if we'd then move the heap symbol
logic into a checker, or if it would still stay in Core.
In the short term, I guess the best idea is to duplicate some of
the checks (or refactor them to a helper function
somewhere...though probably/not/ into AST) and then conjure a heap
symbol if we know we can.
Failed to find any suitable place other then AST :) Eventually
noticed, that actually only a single check should be duplicated.
Decided to leave the wide comment added when I tried to find the
proper place for isStandardNewDelete().
New fix attached.
I don't think this is safe -- what if the user has custom libraries in
their system include path? We can really only assume heap allocation
for the implicit case and for ::operator new(std::size_t) and
::operator new(std::size_t, std::nothrow_t).
However, I think just checking "global namespace" might be a good
enough approximation. If the user overrides one of the default
allocators, it ought to behave like the real one, so we'd only be
messing up if they had a new global "allocate from pool" or something.
Also, in theory we'd have to check placement new, but that's handled
specially below anyway.
Updated the logic, improved test coverage.
Evolved one more problem to solve: if overloaded standard operator new
is defined and is called as a function, then it is not recognized as
overloaded operator for some reason. Tests testOpNewArray() and
testOpNew() in NewDelete-custom.cpp cover these issue.
Evolved another issue, that I first thought to be related to case 1),
here is the minimal test:
struct S {
int **p;
};
void testOk(S &s) {
new(s.p) (int*)(new int); // false-positive leak
}
void testLeak() {
S s;
new(s.p) (int*)(new int); // real leak
}
Ha. I would guess this is because VisitCXXNewExpr calls directly
to bindLoc instead of going through evalBind, and so we miss the
pointer-escape-on-bind callback. I can reproduce this with the
existing MallocChecker by changing the inner 'new' to a malloc.
Haven't addressed these yet. The leak is reported for cases of the
form 'small_vector.push_back(new Something)', where push_back() uses
placement new to store data.
I'll get to this case soon if you don't.
Jordan
--
Anton
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 177966)
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy)
@@ -457,6 +457,10 @@
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.
bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
ASTContext &C) const {
if (!FD)
@@ -467,9 +471,8 @@
Kind != OO_Delete && Kind != OO_Array_Delete)
return false;
- // Skip custom new operators.
- if (!FD->isImplicit() &&
- !C.getSourceManager().isInSystemHeader(FD->getLocStart()))
+ // Skip all operator new/delete methods (including static ones).
+ if (isa<CXXMethodDecl>(FD))
return false;
// Return true if tested operator is a standard placement nothrow operator.
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (revision 177966)
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (working copy)
@@ -278,9 +278,29 @@
unsigned blockCount = currBldrCtx->blockCount();
const LocationContext *LCtx = Pred->getLocationContext();
- DefinedOrUnknownSVal symVal = svalBuilder.conjureSymbolVal(0, CNE, LCtx,
- CNE->getType(),
- blockCount);
+ DefinedOrUnknownSVal symVal = UnknownVal();
+ FunctionDecl *FD = CNE->getOperatorNew();
+
+ bool IsStandardGlobalOpNewFunction = false;
+ if (FD && !isa<CXXMethodDecl>(FD) && !FD->isVariadic()) {
+ if (FD->getNumParams() == 2) {
+ QualType T = FD->getParamDecl(1)->getType();
+ if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
+ // NoThrow placement new behaves as a standard new.
+ IsStandardGlobalOpNewFunction = II->getName().equals("nothrow_t");
+ }
+ else
+ // Placement forms are considered non-standard.
+ IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
+ }
+
+ // We assume all standard global 'operator new' functions allocate memory in
+ // heap.
+ symVal = IsStandardGlobalOpNewFunction
+ ? svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount)
+ : svalBuilder.conjureSymbolVal(0, CNE, LCtx, CNE->getType(),
+ blockCount);
+
ProgramStateRef State = Pred->getState();
CallEventManager &CEMgr = getStateManager().getCallEventManager();
@@ -296,7 +316,6 @@
// is not declared as non-throwing, failures /must/ be signalled by
// exceptions, and thus the return value will never be NULL.
// C++11 [basic.stc.dynamic.allocation]p3.
- FunctionDecl *FD = CNE->getOperatorNew();
if (FD && getContext().getLangOpts().CXXExceptions) {
QualType Ty = FD->getType();
if (const FunctionProtoType *ProtoType = Ty->getAs<FunctionProtoType>())
Index: test/Analysis/NewDelete-checker-test.mm
===================================================================
--- test/Analysis/NewDelete-checker-test.mm (revision 177966)
+++ test/Analysis/NewDelete-checker-test.mm (working copy)
@@ -11,70 +11,54 @@
// check for leaks
//------------------
-void testGlobalExprNewBeforeOverload1() {
- int *p = new int;
+//----- Standard non-placement operators
+void testGlobalOpNew() {
+ void *p = operator new(0);
} // expected-warning{{Memory is never released; potential leak}}
-void testGlobalExprNewBeforeOverload2() {
- int *p = ::new int;
+void testGlobalOpNewArray() {
+ void *p = operator new[](0);
} // expected-warning{{Memory is never released; potential leak}}
-void testGlobalOpNewBeforeOverload() {
- void *p = operator new(0);
+void testGlobalNewExpr() {
+ int *p = new int;
} // expected-warning{{Memory is never released; potential leak}}
-void testMemIsOnHeap() {
- int *p = new int;
- if (global != p)
- global = p;
+void testGlobalNewExprArray() {
+ int *p = new int[0];
} // expected-warning{{Memory is never released; potential leak}}
-//FIXME: currently a memory region for 'new' is not a heap region, that lead to
-//false-positive 'memory leak' ('global != p' is not evaluated to true and 'p'
-//does not escape)
-void *operator new(std::size_t);
-void *operator new(std::size_t, double d);
-void *operator new[](std::size_t);
-void *operator new[](std::size_t, double d);
+//----- Standard nothrow placement operators
+void testGlobalNoThrowPlacementOpNewBeforeOverload() {
+ void *p = operator new(0, std::nothrow);
+} // expected-warning{{Memory is never released; potential leak}}
-void testExprPlacementNew() {
- int i;
- int *p1 = new(&i) int; // no warn - standard placement new
+void testGlobalNoThrowPlacementExprNewBeforeOverload() {
+ int *p = new(std::nothrow) int;
+} // expected-warning{{Memory is never released; potential leak}}
- int *p2 = new(1.0) int; // no warn - overloaded placement new
- int *p3 = new (std::nothrow) int;
-} // expected-warning{{Memory is never released; potential leak}}
-
-void testExprPlacementNewArray() {
+//----- Standard pointer placement operators
+void testGlobalPointerPlacementNew() {
int i;
- int *p1 = new(&i) int[1]; // no warn - standard placement new[]
- int *p2 = new(1.0) int[1]; // no warn - overloaded placement new[]
+ void *p1 = operator new(0, &i); // no warn
- int *p3 = new (std::nothrow) int[1];
-} // expected-warning{{Memory is never released; potential leak}}
+ void *p2 = operator new[](0, &i); // no warn
-void testCustomOpNew() {
- void *p = operator new(0); // no warn - call to a custom new
-}
+ int *p3 = new(&i) int; // no warn
-void testGlobalExprNew() {
- void *p = ::new int; // no warn - call to a custom new
+ int *p4 = new(&i) int[0]; // no warn
}
-void testCustomExprNew() {
- int *p = new int; // no warn - call to a custom new
+//----- Other cases
+void testNewMemoryIsInHeap() {
+ int *p = new int;
+ if (global != p) // condition is always true as 'p' wraps a heap region that
+ // is different from a region wrapped by 'global'
+ global = p; // pointer escapes
}
-void testGlobalExprNewArray() {
- void *p = ::new int[1]; // no warn - call to a custom new
-}
-
-void testOverloadedExprNewArray() {
- int *p = new int[1]; // no warn - call to a custom new
-}
-
//---------------
// other checks
//---------------
@@ -121,22 +105,30 @@
// malloc()/free() are subjects of unix.Malloc and unix.MallocWithAnnotations
void testMallocFreeNoWarn() {
int i;
- free(&i); // no-warning
+ free(&i); // no warn
int *p1 = (int *)malloc(sizeof(int));
- free(++p1); // no-warning
+ free(++p1); // no warn
int *p2 = (int *)malloc(sizeof(int));
free(p2);
- free(p2); // no-warning
+ free(p2); // no warn
- int *p3 = (int *)malloc(sizeof(int)); // no-warning
+ int *p3 = (int *)malloc(sizeof(int)); // no warn
}
-void testFreeNewed() {
+//----- Test free standard new
+void testFreeOpNew() {
+ void *p = operator new(0);
+ free(p);
+} // expected-warning{{Memory is never released; potential leak}}
+// FIXME: Pointer should escape
+
+void testFreeNewExpr() {
int *p = new int;
- free(p); // pointer escaped, no-warning
-}
+ free(p);
+} // expected-warning{{Memory is never released; potential leak}}
+// FIXME: Pointer should escape
void testObjcFreeNewed() {
int *p = new int;
Index: test/Analysis/NewDelete-custom.cpp
===================================================================
--- test/Analysis/NewDelete-custom.cpp (revision 0)
+++ test/Analysis/NewDelete-custom.cpp (working copy)
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,cplusplus.NewDelete -analyzer-store region -std=c++11 -fblocks -verify %s
+#include "Inputs/system-header-simulator-cxx.h"
+
+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, std::nothrow_t& nothrow) throw() { return allocator(size); }
+void *operator new(std::size_t, double d);
+
+class C {
+public:
+ static void *operator new(std::size_t);
+ void *operator new[](std::size_t);
+};
+
+void testNewMethod() {
+ void *p1 = C::operator new(0); // no warn
+
+ C *p2 = new C; // no warn
+
+ C c;
+ void *p3 = c.operator new[](0); // no warn
+
+ C *p4 = new C[0]; // no warn
+
+ C *c5 = ::new C;
+} // expected-warning{{Memory is never released; potential leak}}
+
+void testOpNewArray() {
+ void *p = operator new[](0);
+} //FIXME: expected 'Memory is never released; potential leak'
+
+void testNewExprArray() {
+ int *p = new int[0];
+} // expected-warning{{Memory is never released; potential leak}}
+
+//----- Custom non-placement operators
+void testOpNew() {
+ void *p = operator new(0);
+} //FIXME: expected 'Memory is never released; potential leak'
+
+void testNewExpr() {
+ int *p = new int;
+} // expected-warning{{Memory is never released; potential leak}}
+
+//----- Custom NoThrow placement operators
+void testOpNewNoThrow() {
+ void *p = operator new(0, std::nothrow);
+} // expected-warning{{Memory is never released; potential leak}}
+
+void testNewExprNoThrow() {
+ int *p = new(std::nothrow) int;
+} // expected-warning{{Memory is never released; potential leak}}
+
+//----- Custom placement operators
+void testOpNewPlacement() {
+ void *p = operator new(0, 0.1); // no warn
+}
+
+void testNewExprPlacement() {
+ int *p = new(0.1) int; // no warn
+}
Index: test/Analysis/NewDelete-variadic.cpp
===================================================================
--- test/Analysis/NewDelete-variadic.cpp (revision 0)
+++ test/Analysis/NewDelete-variadic.cpp (working copy)
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,cplusplus.NewDelete -analyzer-store region -std=c++11 -fblocks -verify %s
+// expected-no-diagnostics
+
+namespace std {
+ typedef __typeof__(sizeof(int)) size_t;
+}
+
+void *operator new(std::size_t, ...);
+void *operator new[](std::size_t, ...);
+
+void testGlobalCustomVariadicNew() {
+ void *p1 = operator new(0); // no warn
+
+ void *p2 = operator new[](0); // no warn
+
+ int *p3 = new int; // no warn
+
+ int *p4 = new int[0]; // no warn
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
