[NFC] Fix potential for use-after-free in DumpModuleInfoAction

[Fznamznon]

Summary

Since each DumpModuleInfoAction can now contain a pointer to a
raw_ostream, saving there a poiter that owned by a local unique_ptr
may cause use-after-free. Clarify ownership and save a shared_ptr
inside of DumpModuleInfoAction instead.
Found by static analyzer.

[kastiglione]

I understand the potential for a use after free, since OutputStream is a raw pointer, but I don't understand the fix.

[Fznamznon]

In D146412#4220021, @kastiglione wrote:

I understand the potential for a use after free, since OutputStream is a raw pointer, but I don't understand the fix.

Okay, probably commit message is a little confusing. The potential for a use after free is that OutputStream is a raw pointer as you already noted, but it is also accessible outside of DumpModuleInfoAction::ExecuteAction. Before the patch there was the case where result of local unique_ptr::get was saved there. When the function is exited, unique_ptr frees the memory it holds, making OutputStream poiniting to freed memory. Since OutputStream can be easily accessed outside of DumpModuleInfoAction::ExecuteAction, hence the potential for use-after-free. I wasn't sure if assigning nullptr to OutputStream at the end of DumpModuleInfoAction::ExecuteAction was a good idea, so I just added code avoiding saving result of unique_ptr::get to OutputStream .

[aaron.ballman]

In D146412#4223272, @Fznamznon wrote:

In D146412#4220021, @kastiglione wrote:

I understand the potential for a use after free, since OutputStream is a raw pointer, but I don't understand the fix.

Okay, probably commit message is a little confusing. The potential for a use after free is that OutputStream is a raw pointer as you already noted, but it is also accessible outside of DumpModuleInfoAction::ExecuteAction. Before the patch there was the case where result of local unique_ptr::get was saved there. When the function is exited, unique_ptr frees the memory it holds, making OutputStream poiniting to freed memory. Since OutputStream can be easily accessed outside of DumpModuleInfoAction::ExecuteAction, hence the potential for use-after-free. I wasn't sure if assigning nullptr to OutputStream at the end of DumpModuleInfoAction::ExecuteAction was a good idea, so I just added code avoiding saving result of unique_ptr::get to OutputStream .

Hmmm, but this looks to be the only place DumpModuleInfoAction will initialize OutputStream to a nonnull value, so that seems to be a pretty significant change in behavior.

Should OutputStream be made into a shared_ptr so we can clarify the memory ownership instead of leaving it as a raw pointer? That should also address the use-after-free.

[Fznamznon]

Should OutputStream be made into a shared_ptr so we can clarify the memory ownership instead of leaving it as a raw pointer? That should also address the use-after-free.

Well, I was thinking about that and wasn't sure about it either. https://reviews.llvm.org/D129456 added initialization of OutputStream from the outside. When last shared_ptr is destroyed, it destroys undelying object as well. In case underlying object came from the outside in a form of raw pointer, we may end up accidentally deleting it when DumpModuleInfoAction is destroyed.

[aaron.ballman]

In D146412#4227644, @Fznamznon wrote:

Should OutputStream be made into a shared_ptr so we can clarify the memory ownership instead of leaving it as a raw pointer? That should also address the use-after-free.

Well, I was thinking about that and wasn't sure about it either. https://reviews.llvm.org/D129456 added initialization of OutputStream from the outside. When last shared_ptr is destroyed, it destroys undelying object as well. In case underlying object came from the outside in a form of raw pointer, we may end up accidentally deleting it when DumpModuleInfoAction is destroyed.

Yeah, exposing that data member was not the cleanest way to implement this functionality. This does fix the issue, but it keeps the unclean design -- I wonder if we could make that member private and use a constructor to specify it, then we can enforce the shared ownership property. (If this suggestion turns out to be a slog, then I'm okay with the current approach as well.)

[Fznamznon]

Use private shared_ptr in DumpModuleInfoAction

[aaron.ballman]

This looks much better to me, thank you! LGTM with one small nit for safety.

clang/include/clang/Frontend/FrontendActions.h

191

A bit of extra safety around implicit conversions.

[Fznamznon]

Add a little bit of safety.

[tahonermann]

lldb/source/Commands/CommandObjectTarget.cpp

2182–2187

Use of std::shared_ptr with a deleter that doesn't do anything is unusual; I think this deserves a comment.

[Fznamznon]

Add comment explaining custom deleter.

lldb/source/Commands/CommandObjectTarget.cpp

2182–2187

Sure, done.

[tahonermann]

Thank you, Mariya! Looks good to me!

[Fznamznon]

Rebase.

[Fznamznon]

The test fail is unrelated, seem to be broken by https://reviews.llvm.org/D146811 .

clang/include/clang/Frontend/FrontendActions.h

 public:
   ~SyntaxOnlyAction() override;
   bool hasCodeCompletionSupport() const override { return true; }
 };
 
 /// Dump information about the given module file, to be used for
 /// basic debugging and discovery.
 class DumpModuleInfoAction : public ASTFrontendAction {
-public:
   // Allow other tools (ex lldb) to direct output for their use.
-  llvm::raw_ostream *OutputStream = nullptr;
+  std::shared_ptr<llvm::raw_ostream> OutputStream;
 
 protected:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
   bool BeginInvocation(CompilerInstance &CI) override;
   void ExecuteAction() override;
 
 public:
+  DumpModuleInfoAction() = default;
+  explicit DumpModuleInfoAction(std::shared_ptr<llvm::raw_ostream> Out)
+      : OutputStream(Out) {}
   bool hasPCHSupport() const override { return false; }
   bool hasASTFileSupport() const override { return true; }
   bool hasIRSupport() const override { return false; }
   bool hasCodeCompletionSupport() const override { return false; }
 };
 
 class VerifyPCHAction : public ASTFrontendAction {
 protected:

clang/lib/Frontend/FrontendActions.cpp

case Module::PrivateModuleFragment:
     return "Private Module Fragment";
   }
   llvm_unreachable("unknown module kind!");
 }
 
 void DumpModuleInfoAction::ExecuteAction() {
   assert(isCurrentFileAST() && "dumping non-AST?");
   // Set up the output file.
-  std::unique_ptr<llvm::raw_fd_ostream> OutFile;
   CompilerInstance &CI = getCompilerInstance();
   StringRef OutputFileName = CI.getFrontendOpts().OutputFile;
   if (!OutputFileName.empty() && OutputFileName != "-") {
     std::error_code EC;
-    OutFile.reset(new llvm::raw_fd_ostream(OutputFileName.str(), EC,
-                                           llvm::sys::fs::OF_TextWithCRLF));
-    OutputStream = OutFile.get();
+    OutputStream.reset(new llvm::raw_fd_ostream(
+        OutputFileName.str(), EC, llvm::sys::fs::OF_TextWithCRLF));
   }
   llvm::raw_ostream &Out = OutputStream ? *OutputStream : llvm::outs();
 
   Out << "Information for module file '" << getCurrentFile() << "':\n";
   auto &FileMgr = CI.getFileManager();
   auto Buffer = FileMgr.getBufferForFile(getCurrentFile());
   StringRef Magic = (*Buffer)->getMemBufferRef().getBuffer();
   bool IsRaw = (Magic.size() >= 4 && Magic[0] == 'C' && Magic[1] == 'P' &&

lldb/source/Commands/CommandObjectTarget.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     }
 
     clang::CompilerInstance compiler;
     compiler.createDiagnostics();
 
     const char *clang_args[] = {"clang", pcm_path};
     compiler.setInvocation(clang::createInvocation(clang_args));
 
-    clang::DumpModuleInfoAction dump_module_info;
-    dump_module_info.OutputStream = &result.GetOutputStream().AsRawOstream();
+    // Pass empty deleter to not attempt to free memory that was allocated
+    // outside of the current scope, possibly statically.
+    std::shared_ptr<llvm::raw_ostream> Out(
+        &result.GetOutputStream().AsRawOstream(), [](llvm::raw_ostream *) {});
+    clang::DumpModuleInfoAction dump_module_info(Out);
     // DumpModuleInfoAction requires ObjectFilePCHContainerReader.
     compiler.getPCHContainerOperations()->registerReader(
         std::make_unique<clang::ObjectFilePCHContainerReader>());
 
     if (compiler.ExecuteAction(dump_module_info))
       result.SetStatus(eReturnStatusSuccessFinishResult);
 
     return result.Succeeded();
   }