[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 7, 2014, at 11:56 AM, Nico Weber <thakis at chromium.org> wrote:

> Here's a patch that fixes the leak. The approach is to give ASTReader
> a DeleteListener bool, since different codepaths need different
> ownership semantics. (I explored two other options that didn't work
> out, see comments on PR19560.)

Did you consider using a ref-counted pointer ?

> 
> Can people live with this patch?
> 
> On Sat, May 3, 2014 at 10:01 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> 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.
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
> <clang-deserialisten.patch>