[cfe-dev] [PATCH 1/1] StaticAnalyzer: fix memleak in CFRefCount

[Ted Kremenek]

Hi Jiri,

I think what you are seeing is code in transition.

BugTypes were previously deallocated by the BugReporter class.  Now they aren't, because Checkers can outlive a BugReporter (since Checkers are now managed by CheckerManager).  I think the right approach is to actually reference count BugTypes if we are concerned about BugReporter objects outliving a Checker object.  Alternatively, we can have checkers own them, managing them with an llvm::OwningPtr<>.  Note that this leak is not actually a big deal right now since Checkers stay persistent for the for the analysis of an entire translation unit (so we're not leaking on every function analyzed), but I agree we should clean this up.

Ted

On Apr 6, 2011, at 4:59 AM, Jiri Slaby wrote:

> CFRefCount doesn't free alocated bug types. Do this in the destructor
> if we allocated them in CherkerRegister.
> 
> Note that this approach is racy. If somebody holds the old immutable
> map and iterates over that, they will encounter freed memory. The
> question is whether we want is race-free?
> 
> Valgrind otherwise complains like:
> 172 (40 direct, 132 indirect) bytes in 1 blocks are definitely lost in loss record 27 of 27
>    at 0x4C26337: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>    by 0x16DF42E: (anonymous namespace)::CFRefCount::RegisterChecks(clang::ento::ExprEngine&) (in /l/latest/r
>    by 0x1711DC3: clang::ento::ExprEngine::ExprEngine(clang::ento::AnalysisManager&, clang::ento::TransferFun
>    by 0x16323BC: ActionExprEngine((anonymous namespace)::AnalysisConsumer&, clang::ento::AnalysisManager&, c
> ...
> 
> Signed-off-by: Jiri Slaby <jirislaby at gmail.com>
> Cc: Ted Kremenek <kremenek at apple.com>
> ---
> .../StaticAnalyzer/Core/BugReporter/BugReporter.h  |    1 +
> lib/StaticAnalyzer/Core/BugReporter.cpp            |    4 +++
> lib/StaticAnalyzer/Core/CFRefCount.cpp             |   23 +++++++++++++++++++-
> 3 files changed, 27 insertions(+), 1 deletions(-)
> 
> diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
> index 7c3c6bf..0d758d0 100644
> --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
> +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
> @@ -329,6 +329,7 @@ public:
>         llvm::SmallVectorImpl<BugReport *> &bugReports) {}
> 
>   void Register(BugType *BT);
> +  void Unregister(BugType *BT);
> 
>   void EmitReport(BugReport *R);
> 
> diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp
> index 74cd359..3d9e684 100644
> --- a/lib/StaticAnalyzer/Core/BugReporter.cpp
> +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp
> @@ -1618,6 +1618,10 @@ void BugReporter::Register(BugType *BT) {
>   BugTypes = F.add(BugTypes, BT);
> }
> 
> +void BugReporter::Unregister(BugType *BT) {
> +  BugTypes = F.remove(BugTypes, BT);
> +}
> +
> void BugReporter::EmitReport(BugReport* R) {
>   // Compute the bug report's hash to determine its equivalence class.
>   llvm::FoldingSetNodeID ID;
> diff --git a/lib/StaticAnalyzer/Core/CFRefCount.cpp b/lib/StaticAnalyzer/Core/CFRefCount.cpp
> index 416e120..c763c07 100644
> --- a/lib/StaticAnalyzer/Core/CFRefCount.cpp
> +++ b/lib/StaticAnalyzer/Core/CFRefCount.cpp
> @@ -1691,7 +1691,28 @@ public:
>       leakWithinFunction(0), leakAtReturn(0), overAutorelease(0),
>       returnNotOwnedForOwned(0), BR(0) {}
> 
> -  virtual ~CFRefCount() {}
> +  virtual ~CFRefCount() {
> +    if (BR) {
> +      /* if we have BR, we have also the types */
> +      BR->Unregister(useAfterRelease);
> +      BR->Unregister(releaseNotOwned);
> +      BR->Unregister(deallocGC);
> +      BR->Unregister(deallocNotOwned);
> +      BR->Unregister(leakWithinFunction);
> +      BR->Unregister(leakAtReturn);
> +      BR->Unregister(overAutorelease);
> +      BR->Unregister(returnNotOwnedForOwned);
> +
> +      delete useAfterRelease;
> +      delete releaseNotOwned;
> +      delete deallocGC;
> +      delete deallocNotOwned;
> +      delete leakWithinFunction;
> +      delete leakAtReturn;
> +      delete overAutorelease;
> +      delete returnNotOwnedForOwned;
> +    }
> +  }
> 
>   void RegisterChecks(ExprEngine &Eng);
> 
> -- 
> 1.7.4.2
> 
>