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


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