[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]

I went ahead and landed this patch as-is in r208277 with a somewhat length
commit message. I'm happy to address any feedback anyone has after reading
that message, just reply to the commit :-)


On Wed, May 7, 2014 at 8:11 PM, Nico Weber <thakis at chromium.org> wrote:

> On Wed, May 7, 2014 at 7:38 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> On Wed, May 7, 2014 at 6:34 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>wrote:
>>
>>> 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 ?
>>>
>>
>> Sure, if you prefer that that'd do the trick too. It seemed more
>> intrusive to me.
>>
>
> …after actually trying this: A ref-counted ptr would mean that the
> deserialization listeners have to be allocated on the heap, and many of
> them currently aren't.
>
>
>>
>>
>>>
>>> >
>>> > 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>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140507/2b9fc366/attachment.html>