[cfe-dev] [Static Analyzer] Retain count checker does not warn about parameters that might leak
Devin Coughlin via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 15 13:40:37 PDT 2016
- cfe-dev, + cfe-commits
Hi Tobias,
This is a great start. Thanks for the patch!
> Not sure I know how to use the analyzer config flags yet. My patch
> currently also still causes a segfault on Analysis/retain-release.m
The segfault is because the diagnostics machinery expects a declaration with body for the uniquing declaration (See below).
> and
> changes a test case, where I am not sure it should.
>
> I probably need some time to get this tested and get more confidence in
> what it is doing. I will then put it up for review and we can see how to
> add an analyzer-config flag.
You can take a look at the MallocChecker and the malloc-annotations.c test to see how analyzer-config flags work.
Also, in the future, it is probably best to submit patches on Phabricator: <https://reviews.llvm.org <http://reviews.llvm.org/>> This makes it easier for multiple people to comment on the same patch.
> On Sep 8, 2016, at 2:15 AM, Tobias Grosser <tobias at grosser.es> wrote:
>
> And here a slightly more cleaned up version of my patch, just for
> reference.
>
> Best,
> Tobias
> <0001-RetainCountChecker-Also-check-function-parameters-fo.patch>
> From 19654ebf904c11e3fa5d7f8c938d4a0bb863fbf0 Mon Sep 17 00:00:00 2001
> From: Tobias Grosser <tobias at grosser.es>
> Date: Thu, 8 Sep 2016 09:38:36 +0200
> Subject: [PATCH] RetainCountChecker: Also check function parameters for leaks
>
> ---
> lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp | 87 +++++++++++++++++++---
> test/Analysis/retain-release-function-parameters.m | 26 +++++++
> test/Analysis/retain-release-gc-only.m | 2 +-
> 3 files changed, 105 insertions(+), 10 deletions(-)
> create mode 100644 test/Analysis/retain-release-function-parameters.m
>
> diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
> index d445e91..71035a0 100644
> --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
> +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
> @@ -1823,6 +1823,13 @@ namespace {
>
> class CFRefLeakReport : public CFRefReport {
> const MemRegion* AllocBinding;
> + const Stmt *AllocStmt;
It looks like neither of these instance variables are referred to after object construction. Do they really need to be ivars?
> +
> + void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
> + void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
> + void createDescription(CheckerContext &Ctx,
> + bool GCEnabled, bool IncludeAllocationLine);
> +
> public:
> CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, bool GCEnabled,
> const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,
> @@ -2388,13 +2395,25 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
> return llvm::make_unique<PathDiagnosticEventPiece>(L, os.str());
> }
>
> -CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
> - bool GCEnabled, const SummaryLogTy &Log,
> - ExplodedNode *n, SymbolRef sym,
> - CheckerContext &Ctx,
> - bool IncludeAllocationLine)
> - : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {
> +void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {
> + const SourceManager& SMgr = Ctx.getSourceManager();
> +
> + if (!sym->getOriginRegion())
> + return;
>
Some style notes: In LLVM style we typically use ‘auto *’ for pointers to make it clear there is not an
expensive copy. For example: 'auto *PDecl = …’
We also try to avoided nested indentation where possible. So, for example:
if (!Region)
return;
> + auto Region = dyn_cast<DeclRegion>(sym->getOriginRegion());
> + if (Region) {
> + auto PDecl = Region->getDecl();
> + if (PDecl && isa<ParmVarDecl>(PDecl)) {
I think it is worth adding a comment here explaining that the location
of the leak is the location of the parameter annotated as consumed.
> + auto ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr);
> + Location = ParamLocation;
> + UniqueingLocation = ParamLocation;
> + UniqueingDecl = PDecl;
Rather than using PDecl here, you can use the declaration for the containing function:
UniqueingDecl = Ctx.getLocationContext()->getDecl();
This will avoid the crash when flushing diagnostics.
> + }
> + }
> +}
> +
> +void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) {
> // 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
> @@ -2418,8 +2437,12 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
> // FIXME: This will crash the analyzer if an allocation comes from an
> // implicit call (ex: a destructor call).
> // (Currently there are no such allocations in Cocoa, though.)
> - const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
> - assert(AllocStmt && "Cannot find allocation statement");
> + AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
> +
> + if (!AllocStmt) {
> + AllocBinding = nullptr;
> + return;
> + }
>
> PathDiagnosticLocation AllocLocation =
> PathDiagnosticLocation::createBegin(AllocStmt, SMgr,
> @@ -2430,8 +2453,10 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
> // leaks should be uniqued on the allocation site.
> UniqueingLocation = AllocLocation;
> UniqueingDecl = AllocNode->getLocationContext()->getDecl();
> +}
>
> - // Fill in the description of the bug.
> +void CFRefLeakReport::createDescription(CheckerContext &Ctx,
> + bool GCEnabled, bool IncludeAllocationLine) {
Nit: The indentation here doesn’t follow LLVM guidelines. You might find the ‘clang-format’ tool helpful to match indentation in the rest
of the clang codebase.
> Description.clear();
> llvm::raw_string_ostream os(Description);
> os << "Potential leak ";
> @@ -2446,6 +2471,21 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
> os << " (allocated on line " << SL.getSpellingLineNumber() << ")";
> }
> }
> +}
> +
> +CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,
> + bool GCEnabled, const SummaryLogTy &Log,
> + ExplodedNode *n, SymbolRef sym,
> + CheckerContext &Ctx,
> + bool IncludeAllocationLine)
> + : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {
> +
deriveParamLocation is much, much cheaper than deriveAllocLocation (since the latter walks to the path). Can you
change the ordering to check for the parameter first?
> + deriveAllocLocation(Ctx, sym);
> +
> + if (!AllocBinding)
> + deriveParamLocation(Ctx, sym);
> +
> + createDescription(Ctx, GCEnabled, IncludeAllocationLine);
>
> addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, GCEnabled, Log));
> }
> @@ -2459,6 +2499,7 @@ class RetainCountChecker
> : public Checker< check::Bind,
> check::DeadSymbols,
> check::EndAnalysis,
> + check::BeginFunction,
> check::EndFunction,
> check::PostStmt<BlockExpr>,
> check::PostStmt<CastExpr>,
> @@ -2646,6 +2687,7 @@ public:
> SymbolRef Sym, ProgramStateRef state) const;
>
> void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
> + void checkBeginFunction(CheckerContext &C) const;
> void checkEndFunction(CheckerContext &C) const;
>
> ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym,
> @@ -3854,6 +3896,33 @@ RetainCountChecker::processLeaks(ProgramStateRef state,
>
> return N;
> }
> +void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const {
> + if (!Ctx.inTopFrame())
> + return;
> +
> + const auto *LCtx = Ctx.getLocationContext();
> + const auto *FD = dyn_cast<FunctionDecl>(LCtx->getDecl());
You’ll also need to handle ObjCMethodDecls here. They don’t derive from FunctionDecl.
> +
> + if (!FD)
> + return;
> +
> + ProgramStateRef state = Ctx.getState();
> +
> + for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) {
> + const auto Param = FD->getParamDecl(idx);
> + auto Sym = state->getSVal(state->getRegion(Param, LCtx)).getAsSymbol();
> +
> + QualType Ty = Param->getType();
> + if (Param->hasAttr<CFConsumedAttr>())
One the design philosophies of this checker is that all the knowledge of attributes lives in
the RetainSummary manager, which translates attributes (and naming conventions) into function
summaries which can then be applied (at a call site) and checked (at a return site).
It would be better here to get the function summary with:
getSummaryManager(Ctx).getFunctionSummary(FD).
and then get the ArgEffect for each parameter.
You would then create Owned references for:
DecRef{Msg}{StopTrackingHard} and NotOwned references for all others.
It is important to only add a ref binding if the type is either coreFoundation::isCFObjectRef() or cocoa::isCocoaObjectRef(). Without this, the checker will warn about all parameter types — but ownership discipline needs to be opted in on a per-type basis. Also, isCFObjectRef tells you if the RetEffect:ObjKind should be RetEffect::CF; cocoa::isCocoaObjectRef() tells you if it is RetEffect::ObjC.
> + state = setRefBinding(state, Sym,
> + RefVal::makeOwned(RetEffect::ObjKind::CF, Ty));
> + else
> + state = setRefBinding(state, Sym,
> + RefVal::makeNotOwned(RetEffect::ObjKind::CF, Ty));
> + }
> +
> + Ctx.addTransition(state);
> +}
>
> void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const {
> ProgramStateRef state = Ctx.getState();
> diff --git a/test/Analysis/retain-release-function-parameters.m b/test/Analysis/retain-release-function-parameters.m
> new file mode 100644
> index 0000000..6afec24
> --- /dev/null
> +++ b/test/Analysis/retain-release-function-parameters.m
> @@ -0,0 +1,26 @@
> +// RUN: %clang_cc1 -analyze -analyzer-checker=osx.cocoa.RetainCount %s
> +
> +struct obj;
> +typedef struct obj obj;
> +
> +#define __give __attribute__((cf_returns_retained))
> +#define __take __attribute__((cf_consumed))
> +
> +#define __keep
> +
> +__give obj *alloc();
> +void obj_free(__take obj *o);
> +
> +void taken_and_freed(__take obj *o) {
> + obj_free(o);
> +}
> +
> +void taken_and_not_freed(__take obj *o) { // expected-warning {{Potential leak of an object}}
> +}
> +
> +void kept_and_not_freed(__keep obj *o) {
> +}
> +
> +void kept_and_freed(__keep obj *o) {
> + obj_free(o); // expected-warning {{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
> +}
> diff --git a/test/Analysis/retain-release-gc-only.m b/test/Analysis/retain-release-gc-only.m
> index 26eb6e1..3d3f6e1 100644
> --- a/test/Analysis/retain-release-gc-only.m
> +++ b/test/Analysis/retain-release-gc-only.m
> @@ -224,7 +224,7 @@ CFTypeRef CFMakeCollectable(CFTypeRef cf) ;
>
> static __inline__ __attribute__((always_inline)) id NSMakeCollectable(CFTypeRef
> cf) {
> - return cf ? (id)CFMakeCollectable(cf) : ((void*)0);
> + return cf ? (id)CFMakeCollectable(cf) : ((void*)0); // expected-warning {{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}
> }
>
> //===----------------------------------------------------------------------===//
> --
> 2.7.4
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160915/c7f7138a/attachment-0001.html>
More information about the cfe-commits
mailing list