[LLDB][Minidump] Minidump erase file on failure

[Jlalond]

In #95312 Minidump file creation was moved from being created at the end, to the file being emitted in chunks. This causes some undesirable behavior where the file can still be present after an error has occurred. To resolve this we will now delete the file upon an error.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

In #95312 Minidump file creation was moved from being created at the end, to the file being emitted in chunks. This causes some undesirable behavior where the file can still be present after an error has occurred. To resolve this we will now delete the file upon an error.

---

Full diff: https://github.com/llvm/llvm-project/pull/108259.diff

4 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+13)
• (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h (+3)
• (modified) lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp (+8)
• (modified) lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py (+26)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index edc568a6b47e00..7b7745b2953665 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -1218,3 +1218,16 @@ Status MinidumpFileBuilder::DumpFile() {
 
   return error;
 }
+
+
+void MinidumpFileBuilder::DeleteFile() {
+  Log *log = GetLog(LLDBLog::Object);
+
+  if (m_core_file) {
+    Status error = m_core_file->Close();
+    if (error.Fail()) 
+      LLDB_LOGF(log, "Failed to close minidump file: %s", error.AsCString());
+
+    m_core_file.reset();
+  }
+}
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 71001e26c00e91..2dcabe893b496e 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -115,6 +115,9 @@ class MinidumpFileBuilder {
   // Run cleanup and write all remaining bytes to file
   lldb_private::Status DumpFile();
 
+  // Delete the file if it exists
+  void DeleteFile();
+
 private:
   // Add data to the end of the buffer, if the buffer exceeds the flush level,
   // trigger a flush.
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 5da69dd4f2ce79..282e3fdda2daf6 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -81,28 +81,33 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   if (error.Fail()) {
     LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s",
               error.AsCString());
+    builder.DeleteFile();
     return false;
   };
   error = builder.AddSystemInfo();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
 
   error = builder.AddModuleList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddModuleList failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
   error = builder.AddMiscInfo();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMiscInfo failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
 
   error = builder.AddThreadList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
 
@@ -116,6 +121,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   error = builder.AddExceptions();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddExceptions failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
 
@@ -124,12 +130,14 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   error = builder.AddMemoryList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
 
   error = builder.DumpFile();
   if (error.Fail()) {
     LLDB_LOGF(log, "DumpFile failed: %s", error.AsCString());
+    builder.DeleteFile();
     return false;
   }
 
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 2cbe20ee10b1af..31af96102f9d22 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -493,3 +493,29 @@ def test_save_minidump_custom_save_style_duplicated_regions(self):
 
         finally:
             self.assertTrue(self.dbg.DeleteTarget(target))
+
+    @skipUnlessPlatform(["linux"])
+    def minidump_deleted_on_save_failure(self):
+        """Test that verifies the minidump file is deleted after an error"""
+
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        try:
+            target = self.dbg.CreateTarget(exe)
+            process = target.LaunchSimple(
+                None, None, self.get_process_working_directory()
+            )
+            self.assertState(process.GetState(), lldb.eStateStopped)
+
+            custom_file = self.getBuildArtifact("core.should.be.deleted.custom.dmp")
+            options = lldb.SBSaveCoreOptions()
+            options.SetOutputFile(lldb.SBFileSpec(custom_file))
+            options.SetPluginName("minidump")
+            options.SetStyle(lldb.eSaveCoreCustomOnly)
+            # We set custom only and have no thread list and have no memory.
+            error = process.SaveCore(options)
+            self.assertTrue(error.Fail())
+            self.assertTrue(not os.path.isfile(custom_file))
+
+        finally:
+            self.assertTrue(self.dbg.DeleteTarget(target))

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jeffreytan81]

The logic makes sense.

Implementation wise, instead of calling builder.DeleteFile() in every failure code path, I would suggest creating a RAII class which calls builder.DeleteFile() in destructor. Then you just need to create a single RAII object in this method and without catching every single exit point. This will greatly simplify the code.

[jeffreytan81]

Per our naming convention, let's call this RAII class FileRemoveHolder/DumpFailRemoveHolder or something similar, then the reader immediately know it is a RAII class. The current SaveCoreRequest is not indicating its purpose.

[jeffreytan81]

We should emit visible error message telling end user that saving minidump has failed instead of silently fail and remove minidump file.

[Jlalond]

I'll have to get the patch, it's one of my SBSaveCore API changes, but we now print the error when Plugin Manager returns, we could print twice, one specific for minidump

[jeffreytan81]

I would suggest we added a unit test that checks that we do get an error message if saving minidump failed.

Other than that, this looks good.

[Jlalond]

@jeffreytan81 I'll add a test to ensure we get an error message

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.