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

Nico Weber thakis at chromium.org
Wed May 7 11:56:07 PDT 2014


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.)

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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-deserialisten.patch
Type: application/octet-stream
Size: 15099 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140507/cdf036ac/attachment.obj>


More information about the cfe-commits mailing list