[Lldb-commits] [lldb] [LLDB][Minidump] Minidump erase file on failure (PR #108259)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

<details>
<summary>Changes</summary>

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
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))

``````````

</details>


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