[Lldb-commits] [lldb] [lldb] Download to a unique temporary path in the SymStore locator (PR #214632)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

<details>
<summary>Changes</summary>

The download path was derived only from the key and the PDB name, so two lookups (from different threads) potentially raced the same file path. Avoid this by creating using a unique suffix.

Assisted-by: Claude

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


1 Files Affected:

- (modified) lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp (+26-12) 


``````````diff
diff --git a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp
index eab334e0b8e7a..15a0adbb67670 100644
--- a/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp
+++ b/lldb/source/Plugins/SymbolLocator/SymStore/SymbolLocatorSymStore.cpp
@@ -261,22 +261,32 @@ RequestFileFromSymStoreServerHTTP(llvm::StringRef base_url, llvm::StringRef key,
     return {};
   }
 
-  // Download into a temporary file.
-  llvm::SmallString<128> tmp_file;
-  constexpr bool erase_on_reboot = true;
-  path::system_temp_directory(erase_on_reboot, tmp_file);
-  path::append(tmp_file, llvm::formatv("lldb_symstore_{0}_{1}", key, pdb_name));
-
-  // Server has SymStore directory structure with forward slashes as separators.
-  std::string source_url =
-      llvm::formatv("{0}/{1}/{2}/{1}", base_url, pdb_name, key);
-
   if (!llvm::HTTPClient::isAvailable()) {
     Debugger::ReportWarning(
         "HTTP client is not available for SymStore download");
     return {};
   }
 
+  // Download into a temporary file. The name must be unique: lookups for the
+  // same file can be in flight concurrently.
+  llvm::SmallString<128> tmp_model;
+  constexpr bool erase_on_reboot = true;
+  path::system_temp_directory(erase_on_reboot, tmp_model);
+  path::append(tmp_model,
+               llvm::formatv("lldb_symstore_{0}_{1}.%%%%%%", key, pdb_name));
+
+  llvm::SmallString<128> tmp_file;
+  if (std::error_code ec = fs::createUniqueFile(tmp_model, tmp_file)) {
+    Debugger::ReportWarning(llvm::formatv(
+        "failed to create a temporary file to download '{0}' into: {1}",
+        pdb_name, ec.message()));
+    return {};
+  }
+
+  // Server has SymStore directory structure with forward slashes as separators.
+  std::string source_url =
+      llvm::formatv("{0}/{1}/{2}/{1}", base_url, pdb_name, key);
+
   llvm::HTTPClient client;
   client.setTimeout(
       std::chrono::seconds(GetGlobalPluginProperties().GetTimeout()));
@@ -299,25 +309,29 @@ RequestFileFromSymStoreServerHTTP(llvm::StringRef base_url, llvm::StringRef key,
     Debugger::ReportWarning(
         llvm::formatv("failed to download from SymStore '{0}': {1}", source_url,
                       llvm::toString(std::move(Err))));
+    fs::remove(tmp_file);
     return {};
   }
   if (llvm::Error Err = Handler.commit()) {
     Debugger::ReportWarning(
         llvm::formatv("failed to download from SymStore '{0}': {1}", source_url,
                       llvm::toString(std::move(Err))));
+    fs::remove(tmp_file);
     return {};
   }
 
   unsigned responseCode = client.responseCode();
   switch (responseCode) {
-  case 404:
-    return {}; // file not found
   case 200:
     return FileSpec(tmp_file.str()); // success
+  case 404:
+    fs::remove(tmp_file); // file not found
+    return {};
   default:
     Debugger::ReportWarning(llvm::formatv(
         "failed to download from SymStore '{0}': response code {1}", source_url,
         responseCode));
+    fs::remove(tmp_file);
     return {};
   }
 }

``````````

</details>


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