[analyzer] Adjust the reported variable name in retain count checker

When reporting leaks, we try to attach the leaking object to some
variable, so it's easier to understand.  Before the patch, we always
tried to use the first variable that stored the object in question.
This can get very confusing for the user, if that variable doesn't
contain that object at the moment of the actual leak.  In many cases,
the warning is dismissed as false positive and it is effectively a
false positive when we fail to properly explain the warning to the
user.

This patch addresses the bigest issue in cases like this.  Now we
check if the variable still contains the leaking symbolic object.
If not, we look for the last variable to actually hold it and use
that variable instead.

rdar://76645710

Differential Revision: https://reviews.llvm.org/D100839
This commit is contained in:
Valeriy Savchenko 2021-04-16 21:10:05 +03:00
parent 1dad8c5036
commit 61ae2db2d7
9 changed files with 2034 additions and 54 deletions

View File

@ -13,6 +13,8 @@
#include "RetainCountDiagnostics.h"
#include "RetainCountChecker.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
using namespace clang;
using namespace ento;
@ -337,15 +339,15 @@ public:
class RefLeakReportVisitor : public RefCountReportVisitor {
public:
RefLeakReportVisitor(SymbolRef Sym, const MemRegion *FirstBinding)
: RefCountReportVisitor(Sym), FirstBinding(FirstBinding) {}
RefLeakReportVisitor(SymbolRef Sym, const MemRegion *LastBinding)
: RefCountReportVisitor(Sym), LastBinding(LastBinding) {}
PathDiagnosticPieceRef getEndPath(BugReporterContext &BRC,
const ExplodedNode *N,
PathSensitiveBugReport &BR) override;
private:
const MemRegion *FirstBinding;
const MemRegion *LastBinding;
};
} // end namespace retaincountchecker
@ -614,6 +616,41 @@ static Optional<std::string> describeRegion(const MemRegion *MR) {
return None;
}
using Bindings = llvm::SmallVector<const MemRegion *, 4>;
class VarBindingsCollector : public StoreManager::BindingsHandler {
SymbolRef Sym;
Bindings &Result;
public:
VarBindingsCollector(SymbolRef Sym, Bindings &ToFill)
: Sym(Sym), Result(ToFill) {}
bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *R,
SVal Val) override {
SymbolRef SymV = Val.getAsLocSymbol();
if (!SymV || SymV != Sym)
return true;
if (isa<NonParamVarRegion>(R))
Result.push_back(R);
return true;
}
};
Bindings getAllVarBindingsForSymbol(ProgramStateManager &Manager,
const ExplodedNode *Node, SymbolRef Sym) {
Bindings Result;
VarBindingsCollector Collector{Sym, Result};
while (Result.empty() && Node) {
Manager.iterBindings(Node->getState(), Collector);
Node = Node->getFirstPred();
}
return Result;
}
namespace {
// Find the first node in the current function context that referred to the
// tracked symbol and the memory location that value was stored to. Note, the
@ -740,7 +777,7 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
os << "Object leaked: ";
Optional<std::string> RegionDescription = describeRegion(FirstBinding);
Optional<std::string> RegionDescription = describeRegion(LastBinding);
if (RegionDescription) {
os << "object allocated and stored into '" << *RegionDescription << '\'';
} else {
@ -749,7 +786,7 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
}
// Get the retain count.
const RefVal* RV = getRefBinding(EndN->getState(), Sym);
const RefVal *RV = getRefBinding(EndN->getState(), Sym);
assert(RV);
if (RV->getKind() == RefVal::ErrorLeakReturned) {
@ -790,14 +827,15 @@ RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
" Foundation";
} else if (RV->getObjKind() == ObjKind::OS) {
std::string FuncName = FD->getNameAsString();
os << "whose name ('" << FuncName
<< "') starts with '" << StringRef(FuncName).substr(0, 3) << "'";
os << "whose name ('" << FuncName << "') starts with '"
<< StringRef(FuncName).substr(0, 3) << "'";
}
}
}
} else {
os << " is not referenced later in this execution path and has a retain "
"count of +" << RV->getCount();
"count of +"
<< RV->getCount();
}
return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
@ -819,16 +857,16 @@ RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
addVisitor(std::make_unique<RefCountReportVisitor>(sym));
}
void RefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
const SourceManager& SMgr = Ctx.getSourceManager();
void RefLeakReport::deriveParamLocation(CheckerContext &Ctx) {
const SourceManager &SMgr = Ctx.getSourceManager();
if (!sym->getOriginRegion())
if (!Sym->getOriginRegion())
return;
auto *Region = dyn_cast<DeclRegion>(sym->getOriginRegion());
auto *Region = dyn_cast<DeclRegion>(Sym->getOriginRegion());
if (Region) {
const Decl *PDecl = Region->getDecl();
if (PDecl && isa<ParmVarDecl>(PDecl)) {
if (isa_and_nonnull<ParmVarDecl>(PDecl)) {
PathDiagnosticLocation ParamLocation =
PathDiagnosticLocation::create(PDecl, SMgr);
Location = ParamLocation;
@ -838,8 +876,7 @@ void RefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
}
}
void RefLeakReport::deriveAllocLocation(CheckerContext &Ctx,
SymbolRef sym) {
void RefLeakReport::deriveAllocLocation(CheckerContext &Ctx) {
// Most bug reports are cached at the location where they occurred.
// With leaks, we want to unique them by the location where they were
// allocated, and only report a single path. To do this, we need to find
@ -850,13 +887,13 @@ void RefLeakReport::deriveAllocLocation(CheckerContext &Ctx,
// same SourceLocation.
const ExplodedNode *AllocNode = nullptr;
const SourceManager& SMgr = Ctx.getSourceManager();
const SourceManager &SMgr = Ctx.getSourceManager();
AllocationInfo AllocI =
GetAllocationSite(Ctx.getStateManager(), getErrorNode(), sym);
GetAllocationSite(Ctx.getStateManager(), getErrorNode(), Sym);
AllocNode = AllocI.N;
AllocBinding = AllocI.R;
AllocFirstBinding = AllocI.R;
markInteresting(AllocI.InterestingMethodContext);
// Get the SourceLocation for the allocation site.
@ -866,13 +903,12 @@ void RefLeakReport::deriveAllocLocation(CheckerContext &Ctx,
AllocStmt = AllocNode->getStmtForDiagnostics();
if (!AllocStmt) {
AllocBinding = nullptr;
AllocFirstBinding = nullptr;
return;
}
PathDiagnosticLocation AllocLocation =
PathDiagnosticLocation::createBegin(AllocStmt, SMgr,
AllocNode->getLocationContext());
PathDiagnosticLocation AllocLocation = PathDiagnosticLocation::createBegin(
AllocStmt, SMgr, AllocNode->getLocationContext());
Location = AllocLocation;
// Set uniqieing info, which will be used for unique the bug reports. The
@ -887,7 +923,8 @@ void RefLeakReport::createDescription(CheckerContext &Ctx) {
llvm::raw_string_ostream os(Description);
os << "Potential leak of an object";
Optional<std::string> RegionDescription = describeRegion(AllocBinding);
Optional<std::string> RegionDescription =
describeRegion(AllocBindingToReport);
if (RegionDescription) {
os << " stored into '" << *RegionDescription << '\'';
} else {
@ -897,16 +934,59 @@ void RefLeakReport::createDescription(CheckerContext &Ctx) {
}
}
void RefLeakReport::findBindingToReport(CheckerContext &Ctx,
ExplodedNode *Node) {
if (!AllocFirstBinding)
// If we don't have any bindings, we won't be able to find any
// better binding to report.
return;
// If the original region still contains the leaking symbol...
if (Node->getState()->getSVal(AllocFirstBinding).getAsSymbol() == Sym) {
// ...it is the best binding to report.
AllocBindingToReport = AllocFirstBinding;
return;
}
// At this point, we know that the original region doesn't contain the leaking
// when the actual leak happens. It means that it can be confusing for the
// user to see such description in the message.
//
// Let's consider the following example:
// Object *Original = allocate(...);
// Object *New = Original;
// Original = allocate(...);
// Original->release();
//
// Complaining about a leaking object "stored into Original" might cause a
// rightful confusion because 'Original' is actually released.
// We should complain about 'New' instead.
Bindings AllVarBindings =
getAllVarBindingsForSymbol(Ctx.getStateManager(), Node, Sym);
// While looking for the last var bindings, we can still find
// `AllocFirstBinding` to be one of them. In situations like this,
// it would still be the easiest case to explain to our users.
if (!AllVarBindings.empty() &&
llvm::count(AllVarBindings, AllocFirstBinding) == 0)
// Let's pick one of them at random (if there is something to pick from).
AllocBindingToReport = AllVarBindings[0];
else
AllocBindingToReport = AllocFirstBinding;
}
RefLeakReport::RefLeakReport(const RefCountBug &D, const LangOptions &LOpts,
ExplodedNode *N, SymbolRef Sym,
CheckerContext &Ctx)
: RefCountReport(D, LOpts, N, Sym, /*isLeak=*/true) {
deriveAllocLocation(Ctx, Sym);
if (!AllocBinding)
deriveParamLocation(Ctx, Sym);
deriveAllocLocation(Ctx);
findBindingToReport(Ctx, N);
if (!AllocFirstBinding)
deriveParamLocation(Ctx);
createDescription(Ctx);
addVisitor(std::make_unique<RefLeakReportVisitor>(Sym, AllocBinding));
addVisitor(std::make_unique<RefLeakReportVisitor>(Sym, AllocBindingToReport));
}

View File

@ -68,17 +68,20 @@ public:
};
class RefLeakReport : public RefCountReport {
const MemRegion* AllocBinding;
const Stmt *AllocStmt;
const MemRegion *AllocFirstBinding = nullptr;
const MemRegion *AllocBindingToReport = nullptr;
const Stmt *AllocStmt = nullptr;
PathDiagnosticLocation Location;
// Finds the function declaration where a leak warning for the parameter
// 'sym' should be raised.
void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
// Finds the location where a leak warning for 'sym' should be raised.
void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
void deriveParamLocation(CheckerContext &Ctx);
// Finds the location where the leaking object is allocated.
void deriveAllocLocation(CheckerContext &Ctx);
// Produces description of a leak warning which is printed on the console.
void createDescription(CheckerContext &Ctx);
// Finds the binding that we should use in a leak warning.
void findBindingToReport(CheckerContext &Ctx, ExplodedNode *Node);
public:
RefLeakReport(const RefCountBug &D, const LangOptions &LOpts, ExplodedNode *n,

View File

@ -21946,12 +21946,12 @@
</dict>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Object leaked: object allocated and stored into &apos;foo&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;garply&apos; is not referenced later in this execution path and has a retain count of +1</string>
<key>message</key>
<string>Object leaked: object allocated and stored into &apos;foo&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;garply&apos; is not referenced later in this execution path and has a retain count of +1</string>
</dict>
</array>
<key>description</key><string>Potential leak of an object stored into &apos;foo&apos;</string>
<key>description</key><string>Potential leak of an object stored into &apos;garply&apos;</string>
<key>category</key><string>Memory (Core Foundation/Objective-C/OSObject)</string>
<key>type</key><string>Leak</string>
<key>check_name</key><string>osx.cocoa.RetainCount</string>

View File

@ -25407,12 +25407,12 @@
</dict>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Object leaked: object allocated and stored into &apos;obj&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;second&apos; is not referenced later in this execution path and has a retain count of +1</string>
<key>message</key>
<string>Object leaked: object allocated and stored into &apos;obj&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;second&apos; is not referenced later in this execution path and has a retain count of +1</string>
</dict>
</array>
<key>description</key><string>Potential leak of an object stored into &apos;obj&apos;</string>
<key>description</key><string>Potential leak of an object stored into &apos;second&apos;</string>
<key>category</key><string>Memory (Core Foundation/Objective-C/OSObject)</string>
<key>type</key><string>Leak</string>
<key>check_name</key><string>osx.cocoa.RetainCount</string>
@ -25665,12 +25665,12 @@
</dict>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Object leaked: object allocated and stored into &apos;arr&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;alias&apos; is not referenced later in this execution path and has a retain count of +1</string>
<key>message</key>
<string>Object leaked: object allocated and stored into &apos;arr&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;alias&apos; is not referenced later in this execution path and has a retain count of +1</string>
</dict>
</array>
<key>description</key><string>Potential leak of an object stored into &apos;arr&apos;</string>
<key>description</key><string>Potential leak of an object stored into &apos;alias&apos;</string>
<key>category</key><string>Memory (Core Foundation/Objective-C/OSObject)</string>
<key>type</key><string>Leak</string>
<key>check_name</key><string>osx.cocoa.RetainCount</string>

View File

@ -25476,12 +25476,12 @@
</dict>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Object leaked: object allocated and stored into &apos;obj&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;second&apos; is not referenced later in this execution path and has a retain count of +1</string>
<key>message</key>
<string>Object leaked: object allocated and stored into &apos;obj&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;second&apos; is not referenced later in this execution path and has a retain count of +1</string>
</dict>
</array>
<key>description</key><string>Potential leak of an object stored into &apos;obj&apos;</string>
<key>description</key><string>Potential leak of an object stored into &apos;second&apos;</string>
<key>category</key><string>Memory (Core Foundation/Objective-C/OSObject)</string>
<key>type</key><string>Leak</string>
<key>check_name</key><string>osx.cocoa.RetainCount</string>
@ -25734,12 +25734,12 @@
</dict>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Object leaked: object allocated and stored into &apos;arr&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;alias&apos; is not referenced later in this execution path and has a retain count of +1</string>
<key>message</key>
<string>Object leaked: object allocated and stored into &apos;arr&apos; is not referenced later in this execution path and has a retain count of +1</string>
<string>Object leaked: object allocated and stored into &apos;alias&apos; is not referenced later in this execution path and has a retain count of +1</string>
</dict>
</array>
<key>description</key><string>Potential leak of an object stored into &apos;arr&apos;</string>
<key>description</key><string>Potential leak of an object stored into &apos;alias&apos;</string>
<key>category</key><string>Memory (Core Foundation/Objective-C/OSObject)</string>
<key>type</key><string>Leak</string>
<key>check_name</key><string>osx.cocoa.RetainCount</string>

View File

@ -641,7 +641,7 @@ void test_smart_ptr_leak() {
OSObject *obj = new OSObject; // expected-note{{Operator 'new' returns an OSObject of type 'OSObject' with a +1 retain count}}
{
OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr<OSObject>'}}
// expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
// expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
// expected-note@os_smart_ptr.h:13{{Field 'pointer' is non-null}}
// expected-note@os_smart_ptr.h:13{{Taking true branch}}
// expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}}
@ -653,9 +653,9 @@ void test_smart_ptr_leak() {
// expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}}
// expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}}
// expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}}
// expected-note@-6{{Returning from '~smart_ptr'}}
} // expected-warning{{Potential leak of an object stored into 'obj'}}
// expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}}
// expected-note@-6{{Returning from '~smart_ptr'}}
} // expected-warning{{Potential leak of an object stored into 'p'}}
// expected-note@-1{{Object leaked: object allocated and stored into 'p' is not referenced later in this execution path and has a retain count of +1}}
void test_smart_ptr_no_leak() {
OSObject *obj = new OSObject;

View File

@ -212,7 +212,7 @@ static int Cond;
}
-(id)initY {
self = [super init]; //expected-note {{Method returns an instance of MyObj with a +1 retain count}}
self = [super init]; // expected-note 6 {{Method returns an instance of MyObj with a +1 retain count}}
return self;
}
@ -327,5 +327,74 @@ void CFAutoreleaseUnownedMixed() {
@end
int seed();
@interface LeakReassignmentTests : MyObj
@end
@implementation LeakReassignmentTests
+(void)testLeakAliasSimple {
id Original = [[MyObj alloc] initY]; // expected-note {{Calling 'initY'}}
// expected-note@-1 {{Returning from 'initY'}}
id New = Original;
Original = [[MyObj alloc] initZ];
(void)New;
[Original release]; // expected-warning {{Potential leak of an object stored into 'New'}}
// expected-note@-1 {{Object leaked: object allocated and stored into 'New' is not referenced later in this execution path and has a retain count of +1}}
}
+(void)testLeakAliasChain {
id Original = [[MyObj alloc] initY]; // expected-note {{Calling 'initY'}}
// expected-note@-1 {{Returning from 'initY'}}
id Intermediate = Original;
id New = Intermediate;
Original = [[MyObj alloc] initZ];
(void)New;
[Original release]; // expected-warning {{Potential leak of an object stored into 'New'}}
// expected-note@-1 {{Object leaked: object allocated and stored into 'New' is not referenced later in this execution path and has a retain count of +1}}
}
+(void)log:(id)Obj with:(int)Num {
Num *= 42;
if (Obj )
Num /= 2;
}
+(int)calculate {
int x = 10;
int y = 25;
x += y * x + seed();
return y - x * y;
}
+(void)testLeakAliasDeathInExpr {
id Original = [[MyObj alloc] initY]; // expected-note {{Calling 'initY'}}
// expected-note@-1 {{Returning from 'initY'}}
id New = Original;
Original = [[MyObj alloc] initZ];
[self log:New with:[self calculate]];
[Original release]; // expected-warning {{Potential leak of an object stored into 'New'}}
// expected-note@-1 {{Object leaked: object allocated and stored into 'New' is not referenced later in this execution path and has a retain count of +1}}
}
+(void)testLeakReassign {
id Original = [[MyObj alloc] initY]; // expected-note {{Calling 'initY'}}
// expected-note@-1 {{Returning from 'initY'}}
// TODO: move warning here
Original = [[MyObj alloc] initZ];
[Original release]; // expected-warning {{Potential leak of an object stored into 'Original'}}
// expected-note@-1 {{Object leaked: object allocated and stored into 'Original' is not referenced later in this execution path and has a retain count of +1}}
}
+(void)testLeakReassign:(int)cond {
id Original = [[MyObj alloc] initY]; // expected-note {{Calling 'initY'}}
// expected-note@-1 {{Returning from 'initY'}}
if (cond) // expected-note {{Assuming 'cond' is not equal to 0}}
// expected-note@-1 {{Taking true branch}}
// TODO: move warning here
Original = [[MyObj alloc] initZ];
[Original release]; // expected-warning {{Potential leak of an object stored into 'Original'}}
// expected-note@-1 {{Object leaked: object allocated and stored into 'Original' is not referenced later in this execution path and has a retain count of +1}}
}
@end

View File

@ -2282,7 +2282,7 @@ void useAfterRelease() {
void testAutoreleaseReturnsInput() {
extern CFTypeRef CFCreateSomething();
CFTypeRef obj = CFCreateSomething(); // expected-warning{{Potential leak of an object stored into 'obj'}}
CFTypeRef obj = CFCreateSomething(); // expected-warning{{Potential leak of an object stored into 'second'}}
CFTypeRef second = CFAutorelease(obj);
CFRetain(second);
}
@ -2302,7 +2302,7 @@ void autoreleaseTypedObject() {
}
void autoreleaseReturningTypedObject() {
CFArrayRef arr = CFArrayCreateMutable(0, 10, &kCFTypeArrayCallBacks); // expected-warning{{Potential leak of an object stored into 'arr'}}
CFArrayRef arr = CFArrayCreateMutable(0, 10, &kCFTypeArrayCallBacks); // expected-warning{{Potential leak of an object stored into 'alias'}}
CFArrayRef alias = (CFArrayRef)CFAutorelease((CFTypeRef)arr);
CFRetain(alias);
}