[cfe-commits] r116503 - in /cfe/trunk: include/clang/Driver/CC1Options.td include/clang/Frontend/PreprocessorOptions.h lib/Frontend/CompilerInvocation.cpp lib/Frontend/FrontendAction.cpp

[Argyrios Kyrtzidis]

On May 3, 2014, at 3:55 AM, Chandler Carruth <chandlerc at gmail.com> wrote:

> On Sat, May 3, 2014 at 3:20 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
> First off, sorry for the commit necromancy, but I tried to fix a memory leak here and discovered completely broken ownership semantics that really need to be fixed, and need to be fixed by the person who really understands the intended design here.
> 
> On Thu, Oct 14, 2010 at 1:14 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> Author: akirtzidis
> Date: Thu Oct 14 15:14:18 2010
> New Revision: 116503
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=116503&view=rev
> Log:
> Introduce command line option -dump-deserialized-decls which is used to print the PCH decls that got deserialized, for testing purposes.
> 
> <snip>
>  
> Modified: cfe/trunk/lib/Frontend/FrontendAction.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendAction.cpp?rev=116503&r1=116502&r2=116503&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Frontend/FrontendAction.cpp (original)
> +++ cfe/trunk/lib/Frontend/FrontendAction.cpp Thu Oct 14 15:14:18 2010
> @@ -16,12 +16,43 @@
>  #include "clang/Frontend/CompilerInstance.h"
>  #include "clang/Frontend/FrontendDiagnostic.h"
>  #include "clang/Parse/ParseAST.h"
> +#include "clang/Serialization/ASTDeserializationListener.h"
>  #include "llvm/Support/MemoryBuffer.h"
>  #include "llvm/Support/Timer.h"
>  #include "llvm/Support/ErrorHandling.h"
>  #include "llvm/Support/raw_ostream.h"
>  using namespace clang;
> 
> +namespace {
> +
> +/// \brief Dumps deserialized declarations.
> +class DeserializedDeclsDumper : public ASTDeserializationListener {
> +  ASTDeserializationListener *Previous;
> +
> +public:
> +  DeserializedDeclsDumper(ASTDeserializationListener *Previous)
> +    : Previous(Previous) { }
> +
> +  virtual void DeclRead(serialization::DeclID ID, const Decl *D) {
> +    llvm::outs() << "PCH DECL: " << D->getDeclKindName();
> +    if (const NamedDecl *ND = dyn_cast<NamedDecl>(D))
> +      llvm::outs() << " - " << ND->getNameAsString();
> +    llvm::outs() << "\n";
> +
> +    if (Previous)
> +      Previous->DeclRead(ID, D);
> +  }
> +
> +  virtual void SetReader(ASTReader *Reader) {}
> +  virtual void IdentifierRead(serialization::IdentID ID, IdentifierInfo *II) {}
> +  virtual void TypeRead(serialization::TypeIdx Idx, QualType T) {}
> +  virtual void SelectorRead(serialization::SelectorID iD, Selector Sel) {}
> +  virtual void MacroDefinitionRead(serialization::MacroID,
> +                                   MacroDefinition *MD) {}
> +};
> +
> +} // end anonymous namespace
> +
>  FrontendAction::FrontendAction() : Instance(0) {}
> 
>  FrontendAction::~FrontendAction() {}
> @@ -118,11 +149,15 @@
>      /// Use PCH?
>      if (!CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) {
>        assert(hasPCHSupport() && "This action does not have PCH support!");
> +      ASTDeserializationListener *DeserialListener
> +          = CI.getInvocation().getFrontendOpts().ChainedPCH ?
> +                  Consumer->GetASTDeserializationListener() : 0;
> +      if (CI.getPreprocessorOpts().DumpDeserializedPCHDecls)
> +        DeserialListener = new DeserializedDeclsDumper(DeserialListener);
> 
> This listener leaks. So does the DeserializedDeclsChecker that follows the same pattern. LSan catches this.
> 
> I went to plug the leak by storing the listeners in the FrontendAction if they end up being used, and deallocating them when the action is destroyed. To my great surprise, this caused use-after-free bugs as these listeners are somehow reached *after* the action is destroyed!
> 
> Could you look into this? It should be pretty easy to reproduce. Forward declare the classes (or make them private classes nested within FrontendAction, or use a raw pointer and manually cast back to the derived type prior to deleting) and add unique_ptrs to the action, reseting them with the new listeners, and just wiring up DeserialListener to the raw pointer side. If you're having trouble reproducing, let me know, and I can send you a nascent patch.
> 
> -Chandler
> 
> Just FYI, this is also filed as PR19560.

Thanks for bringing this to my attention! Please attach your patch to the PR.
Unfortunately I'm not going to look into this soon, so I suggest disabling the test, that uses the flag, for LSan.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140503/6c2fc08e/attachment.html>