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

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/9381bc48/attachment.html>