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.
2) The second type of false-positives is illustrated by the
following minimal test:
void f(const int & x);
void test() {
int *p = (int *)malloc(sizeof(int));
f(*p);
} // report false-positive leak
report-218274.html shows how it looks like in reality.
Haven't addressed this yet. Removing 'const' from the declaration
eliminates the leak report.
Interesting. You can't change a const region (and pedantically you
can't free() it either), but you certainly can 'delete' it. (C++11
[expr.delete]p2)
Anna, any thoughts on this? Should these also count as "pointer
escaped" even though their contents have not been invalidated?
I think handling this similarly to pointer escape is the best.
However, I am concerned that if we simply extend pointer escape to
trigger on another "Kind" of escape, all the other users of
pointerEscape will need to know about it (and do nothing for this kind
of escape). How about a new checker callback, which will rely on the
same core code as _checkPointerEscape? This way the checker developers
would not need to know about this special case, and we can reuse all
the code that determines when the escape should be triggered.
As for me it would be better to leave the single callback as this is
just another type of pointer escape, if I understand correctly. Have no
other arguments for this :)
In addition, the "Kind" approach is relatively new, so hopefully a few
users of pointerEscape be affected.
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
}
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.
--
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;
}
+// \brief Tells if the callee is one of the following:
+// 1) Standard library non-placement operator new/delete.
+// 2) Standard library placement operator with the single placement argument
+// of type std::nothrow_t.
bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
ASTContext &C) const {
if (!FD)
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (revision 177966)
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (working copy)
@@ -278,9 +278,19 @@
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();
+
+ if (FD && (FD->isImplicit() ||
+ getContext().getSourceManager().isInSystemHeader(FD->getLocStart())))
+ // We know all standard library 'new' operators to allocate memory on the
+ // heap.
+ symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
+ else
+ // Can't be sure that the memory is heap allocated.
+ symVal = svalBuilder.conjureSymbolVal(0, CNE, LCtx, CNE->getType(),
+ blockCount);
+
ProgramStateRef State = Pred->getState();
CallEventManager &CEMgr = getStateManager().getCallEventManager();
@@ -296,7 +306,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>())
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
