From: Argyrios Kyrtzidis Date: Thu, 27 Feb 2014 04:11:59 +0000 (+0000) Subject: [ASTUnit] Fix use-after-free bug in ASTUnit::getMainBufferWithPrecompiledPreamble(). X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0242440d9fdc32a81fa397445d5a8b599875e446;p=clang [ASTUnit] Fix use-after-free bug in ASTUnit::getMainBufferWithPrecompiledPreamble(). With r197755 we started reading the contents of buffer file entries, but the buffers may point to ASTReader blobs that have been disposed. Fix this by having the CompilerInstance object keep a reference to the ASTReader as well as having the ASTContext keep reference to the ExternalASTSource. This was very difficult to construct a test case for. rdar://16149782 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@202346 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/ASTContext.h b/include/clang/AST/ASTContext.h index 2152390f7a..6ed7c0804b 100644 --- a/include/clang/AST/ASTContext.h +++ b/include/clang/AST/ASTContext.h @@ -416,7 +416,7 @@ public: SelectorTable &Selectors; Builtin::Context &BuiltinInfo; mutable DeclarationNameTable DeclarationNames; - OwningPtr ExternalSource; + IntrusiveRefCntPtr ExternalSource; ASTMutationListener *Listener; /// \brief Contains parents of a node. @@ -811,11 +811,13 @@ public: /// The external AST source provides the ability to load parts of /// the abstract syntax tree as needed from some external storage, /// e.g., a precompiled header. - void setExternalSource(OwningPtr &Source); + void setExternalSource(IntrusiveRefCntPtr Source); /// \brief Retrieve a pointer to the external AST source associated /// with this AST context, if any. - ExternalASTSource *getExternalSource() const { return ExternalSource.get(); } + ExternalASTSource *getExternalSource() const { + return ExternalSource.getPtr(); + } /// \brief Attach an AST mutation listener to the AST context. /// diff --git a/include/clang/AST/ExternalASTSource.h b/include/clang/AST/ExternalASTSource.h index b077426e6a..54e96d99bc 100644 --- a/include/clang/AST/ExternalASTSource.h +++ b/include/clang/AST/ExternalASTSource.h @@ -53,7 +53,7 @@ enum ExternalLoadResult { /// sources can resolve types and declarations from abstract IDs into /// actual type and declaration nodes, and read parts of declaration /// contexts. -class ExternalASTSource { +class ExternalASTSource : public RefCountedBase { /// \brief Whether this AST source also provides information for /// semantic analysis. bool SemaSource; diff --git a/include/clang/Frontend/ASTUnit.h b/include/clang/Frontend/ASTUnit.h index 4a64b24b38..adc920a8c7 100644 --- a/include/clang/Frontend/ASTUnit.h +++ b/include/clang/Frontend/ASTUnit.h @@ -75,7 +75,7 @@ private: IntrusiveRefCntPtr Ctx; IntrusiveRefCntPtr TargetOpts; IntrusiveRefCntPtr HSOpts; - ASTReader *Reader; + IntrusiveRefCntPtr Reader; bool HadModuleLoaderFatalFailure; struct ASTWriterData; diff --git a/include/clang/Frontend/ChainedIncludesSource.h b/include/clang/Frontend/ChainedIncludesSource.h index aa30460040..bc05c078a6 100644 --- a/include/clang/Frontend/ChainedIncludesSource.h +++ b/include/clang/Frontend/ChainedIncludesSource.h @@ -24,13 +24,13 @@ class ChainedIncludesSource : public ExternalSemaSource { public: virtual ~ChainedIncludesSource(); - static ChainedIncludesSource *create(CompilerInstance &CI); + static IntrusiveRefCntPtr create(CompilerInstance &CI); ExternalSemaSource &getFinalReader() const { return *FinalReader; } private: std::vector CIs; - OwningPtr FinalReader; + IntrusiveRefCntPtr FinalReader; protected: diff --git a/include/clang/Frontend/CompilerInstance.h b/include/clang/Frontend/CompilerInstance.h index be1e1eaf3b..c8ffd32624 100644 --- a/include/clang/Frontend/CompilerInstance.h +++ b/include/clang/Frontend/CompilerInstance.h @@ -102,8 +102,8 @@ class CompilerInstance : public ModuleLoader { /// \brief The frontend timer OwningPtr FrontendTimer; - /// \brief Non-owning reference to the ASTReader, if one exists. - ASTReader *ModuleManager; + /// \brief The ASTReader, if one exists. + IntrusiveRefCntPtr ModuleManager; /// \brief The set of top-level modules that has already been loaded, /// along with the module map @@ -454,8 +454,8 @@ public: /// @name Module Management /// { - ASTReader *getModuleManager() const { return ModuleManager; } - void setModuleManager(ASTReader *Reader) { ModuleManager = Reader; } + IntrusiveRefCntPtr getModuleManager() const; + void setModuleManager(IntrusiveRefCntPtr Reader); /// } /// @name Code Completion diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp index 46d0451017..65bb024b2a 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -791,8 +791,8 @@ void ASTContext::AddDeallocation(void (*Callback)(void*), void *Data) { } void -ASTContext::setExternalSource(OwningPtr &Source) { - ExternalSource.reset(Source.take()); +ASTContext::setExternalSource(IntrusiveRefCntPtr Source) { + ExternalSource = Source; } void ASTContext::PrintStats() const { @@ -846,7 +846,7 @@ void ASTContext::PrintStats() const { << NumImplicitDestructors << " implicit destructors created\n"; - if (ExternalSource.get()) { + if (ExternalSource) { llvm::errs() << "\n"; ExternalSource->PrintStats(); } diff --git a/lib/Frontend/ASTUnit.cpp b/lib/Frontend/ASTUnit.cpp index 8d46a8f4a5..93429cba88 100644 --- a/lib/Frontend/ASTUnit.cpp +++ b/lib/Frontend/ASTUnit.cpp @@ -718,8 +718,6 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename, HeaderSearch &HeaderInfo = *AST->HeaderInfo.get(); unsigned Counter; - OwningPtr Reader; - AST->PP = new Preprocessor(PPOpts, AST->getDiagnostics(), AST->ASTFileLangOpts, /*Target=*/0, AST->getSourceManager(), HeaderInfo, @@ -742,21 +740,17 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename, bool disableValid = false; if (::getenv("LIBCLANG_DISABLE_PCH_VALIDATION")) disableValid = true; - Reader.reset(new ASTReader(PP, Context, + AST->Reader = new ASTReader(PP, Context, /*isysroot=*/"", /*DisableValidation=*/disableValid, - AllowPCHWithCompilerErrors)); - - // Recover resources if we crash before exiting this method. - llvm::CrashRecoveryContextCleanupRegistrar - ReaderCleanup(Reader.get()); + AllowPCHWithCompilerErrors); - Reader->setListener(new ASTInfoCollector(*AST->PP, Context, + AST->Reader->setListener(new ASTInfoCollector(*AST->PP, Context, AST->ASTFileLangOpts, AST->TargetOpts, AST->Target, Counter)); - switch (Reader->ReadAST(Filename, serialization::MK_MainFile, + switch (AST->Reader->ReadAST(Filename, serialization::MK_MainFile, SourceLocation(), ASTReader::ARR_None)) { case ASTReader::Success: break; @@ -771,21 +765,14 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename, return NULL; } - AST->OriginalSourceFile = Reader->getOriginalSourceFile(); + AST->OriginalSourceFile = AST->Reader->getOriginalSourceFile(); PP.setCounterValue(Counter); // Attach the AST reader to the AST context as an external AST // source, so that declarations will be deserialized from the // AST file as needed. - ASTReader *ReaderPtr = Reader.get(); - OwningPtr Source(Reader.take()); - - // Unregister the cleanup for ASTReader. It will get cleaned up - // by the ASTUnit cleanup. - ReaderCleanup.unregister(); - - Context.setExternalSource(Source); + Context.setExternalSource(AST->Reader); // Create an AST consumer, even though it isn't used. AST->Consumer.reset(new ASTConsumer); @@ -793,8 +780,7 @@ ASTUnit *ASTUnit::LoadFromASTFile(const std::string &Filename, // Create a semantic analysis object and tell the AST reader about it. AST->TheSema.reset(new Sema(PP, Context, *AST->Consumer)); AST->TheSema->Initialize(); - ReaderPtr->InitializeSema(*AST->TheSema); - AST->Reader = ReaderPtr; + AST->Reader->InitializeSema(*AST->TheSema); // Tell the diagnostic client that we have started a source file. AST->getDiagnostics().getClient()->BeginSourceFile(Context.getLangOpts(),&PP); @@ -1173,7 +1159,7 @@ bool ASTUnit::Parse(llvm::MemoryBuffer *OverrideMainBuffer) { if (OverrideMainBuffer) { std::string ModName = getPreambleFile(this); - TranslateStoredDiagnostics(Clang->getModuleManager(), ModName, + TranslateStoredDiagnostics(Clang->getModuleManager().getPtr(), ModName, getSourceManager(), PreambleDiagnostics, StoredDiagnostics); } diff --git a/lib/Frontend/ChainedIncludesSource.cpp b/lib/Frontend/ChainedIncludesSource.cpp index 442177ec31..ab5c81fb58 100644 --- a/lib/Frontend/ChainedIncludesSource.cpp +++ b/lib/Frontend/ChainedIncludesSource.cpp @@ -62,12 +62,13 @@ ChainedIncludesSource::~ChainedIncludesSource() { delete CIs[i]; } -ChainedIncludesSource *ChainedIncludesSource::create(CompilerInstance &CI) { +IntrusiveRefCntPtr +ChainedIncludesSource::create(CompilerInstance &CI) { std::vector &includes = CI.getPreprocessorOpts().ChainedIncludes; assert(!includes.empty() && "No '-chain-include' in options!"); - OwningPtr source(new ChainedIncludesSource()); + IntrusiveRefCntPtr source(new ChainedIncludesSource()); InputKind IK = CI.getFrontendOpts().Inputs[0].getKind(); SmallVector serialBufs; @@ -137,13 +138,12 @@ ChainedIncludesSource *ChainedIncludesSource::create(CompilerInstance &CI) { serialBufNames.push_back(pchName); - OwningPtr Reader; - - Reader.reset(createASTReader(*Clang, pchName, bufs, serialBufNames, - Clang->getASTConsumer().GetASTDeserializationListener())); + IntrusiveRefCntPtr Reader; + Reader = createASTReader(*Clang, pchName, bufs, serialBufNames, + Clang->getASTConsumer().GetASTDeserializationListener()); if (!Reader) return 0; - Clang->setModuleManager(static_cast(Reader.get())); + Clang->setModuleManager(Reader); Clang->getASTContext().setExternalSource(Reader); } @@ -162,13 +162,13 @@ ChainedIncludesSource *ChainedIncludesSource::create(CompilerInstance &CI) { assert(!serialBufs.empty()); std::string pchName = includes.back() + ".pch-final"; serialBufNames.push_back(pchName); - OwningPtr Reader; - Reader.reset(createASTReader(CI, pchName, serialBufs, serialBufNames)); + IntrusiveRefCntPtr Reader; + Reader = createASTReader(CI, pchName, serialBufs, serialBufNames); if (!Reader) return 0; - source->FinalReader.reset(Reader.take()); - return source.take(); + source->FinalReader = Reader; + return source; } //===----------------------------------------------------------------------===// diff --git a/lib/Frontend/CompilerInstance.cpp b/lib/Frontend/CompilerInstance.cpp index 3fe8f0e82f..cdc4b64af7 100644 --- a/lib/Frontend/CompilerInstance.cpp +++ b/lib/Frontend/CompilerInstance.cpp @@ -104,6 +104,13 @@ void CompilerInstance::setASTConsumer(ASTConsumer *Value) { void CompilerInstance::setCodeCompletionConsumer(CodeCompleteConsumer *Value) { CompletionConsumer.reset(Value); } + +IntrusiveRefCntPtr CompilerInstance::getModuleManager() const { + return ModuleManager; +} +void CompilerInstance::setModuleManager(IntrusiveRefCntPtr Reader) { + ModuleManager = Reader; +} // Diagnostics static void SetUpDiagnosticLog(DiagnosticOptions *DiagOpts, @@ -301,16 +308,16 @@ void CompilerInstance::createPCHExternalASTSource(StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors, void *DeserializationListener){ - OwningPtr Source; + IntrusiveRefCntPtr Source; bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0; - Source.reset(createPCHExternalASTSource(Path, getHeaderSearchOpts().Sysroot, + Source = createPCHExternalASTSource(Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation, AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(), DeserializationListener, Preamble, - getFrontendOpts().UseGlobalModuleIndex)); - ModuleManager = static_cast(Source.get()); + getFrontendOpts().UseGlobalModuleIndex); + ModuleManager = static_cast(Source.getPtr()); getASTContext().setExternalSource(Source); } @@ -1176,9 +1183,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc, getASTContext().setASTMutationListener( getASTConsumer().GetASTMutationListener()); } - OwningPtr Source; - Source.reset(ModuleManager); - getASTContext().setExternalSource(Source); + getASTContext().setExternalSource(ModuleManager); if (hasSema()) ModuleManager->InitializeSema(getSema()); if (hasASTConsumer()) diff --git a/lib/Frontend/FrontendAction.cpp b/lib/Frontend/FrontendAction.cpp index 6cfa12168d..f028a56053 100644 --- a/lib/Frontend/FrontendAction.cpp +++ b/lib/Frontend/FrontendAction.cpp @@ -316,12 +316,11 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI, if (!CI.getPreprocessorOpts().ChainedIncludes.empty()) { // Convert headers to PCH and chain them. - OwningPtr source; - source.reset(ChainedIncludesSource::create(CI)); + IntrusiveRefCntPtr source; + source = ChainedIncludesSource::create(CI); if (!source) goto failure; - CI.setModuleManager(static_cast( - &static_cast(source.get())->getFinalReader())); + CI.setModuleManager(static_cast(&source->getFinalReader())); CI.getASTContext().setExternalSource(source); } else if (!CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) { @@ -361,7 +360,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI, // provides the layouts from that file. if (!CI.getFrontendOpts().OverrideRecordLayoutsFile.empty() && CI.hasASTContext() && !CI.getASTContext().getExternalSource()) { - OwningPtr + IntrusiveRefCntPtr Override(new LayoutOverrideSource( CI.getFrontendOpts().OverrideRecordLayoutsFile)); CI.getASTContext().setExternalSource(Override);