From 5ef24eb2bb44498cf363ff7f8a66a0ac97b86f08 Mon Sep 17 00:00:00 2001
From: Even Rouault <even.rouault@spatialys.com>
Date: Wed, 7 Feb 2024 14:50:34 +0100
Subject: [PATCH] /vsicurl/: fix potential multithreaded crash when downloading
 the same region in parallel and that the download fails

If 2 threads tried to download the same region at the same time, one of
them would wait for the first one to have finished. But if that download
failed, the waiting thread would then wrongly try to unregister the
region in m_oMapRegionInDownload (the first thread has already done
that), resulting in either an assertion in debug mode on
```CPLAssert(oIter != m_oMapRegionInDownload.end())``` in
NotifyStopDownloadRegion() or a crash later when trying to lock
region.oMutex which would be random data.
---
 port/cpl_vsil_curl.cpp     | 48 ++++++++++++++++++++++++++------------
 port/cpl_vsil_curl_class.h |  7 +++---
 2 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/port/cpl_vsil_curl.cpp b/port/cpl_vsil_curl.cpp
index b7eaa7f71d31..9063da67925e 100644
--- a/port/cpl_vsil_curl.cpp
+++ b/port/cpl_vsil_curl.cpp
@@ -1622,35 +1622,41 @@ struct CurrentDownload
     std::string m_osURL{};
     vsi_l_offset m_nStartOffset = 0;
     int m_nBlocks = 0;
-    std::string m_osData{};
-    bool m_bDone = false;
+    std::string m_osAlreadyDownloadedData{};
+    bool m_bHasAlreadyDownloadedData = false;
 
     CurrentDownload(VSICurlFilesystemHandlerBase *poFS, const char *pszURL,
                     vsi_l_offset startOffset, int nBlocks)
         : m_poFS(poFS), m_osURL(pszURL), m_nStartOffset(startOffset),
           m_nBlocks(nBlocks)
     {
-        m_osData = m_poFS->NotifyStartDownloadRegion(m_osURL, m_nStartOffset,
+        auto res = m_poFS->NotifyStartDownloadRegion(m_osURL, m_nStartOffset,
                                                      m_nBlocks);
-        m_bDone = !m_osData.empty();
+        m_bHasAlreadyDownloadedData = res.first;
+        m_osAlreadyDownloadedData = std::move(res.second);
+    }
+
+    bool HasAlreadyDownloadedData() const
+    {
+        return m_bHasAlreadyDownloadedData;
     }
 
     const std::string &GetAlreadyDownloadedData() const
     {
-        return m_osData;
+        return m_osAlreadyDownloadedData;
     }
 
     void SetData(const std::string &osData)
     {
-        CPLAssert(!m_bDone);
-        m_bDone = true;
+        CPLAssert(!m_bHasAlreadyDownloadedData);
+        m_bHasAlreadyDownloadedData = true;
         m_poFS->NotifyStopDownloadRegion(m_osURL, m_nStartOffset, m_nBlocks,
                                          osData);
     }
 
     ~CurrentDownload()
     {
-        if (!m_bDone)
+        if (!m_bHasAlreadyDownloadedData)
             m_poFS->NotifyStopDownloadRegion(m_osURL, m_nStartOffset, m_nBlocks,
                                              std::string());
     }
@@ -1664,7 +1670,19 @@ struct CurrentDownload
 /*                      NotifyStartDownloadRegion()                     */
 /************************************************************************/
 
-std::string VSICurlFilesystemHandlerBase::NotifyStartDownloadRegion(
+/** Indicate intent at downloading a new region.
+ *
+ * If the region is already in download in another thread, then wait for its
+ * completion.
+ *
+ * Returns:
+ * - (false, empty string) if a new download is needed
+ * - (true, region_content) if we have been waiting for a download of the same
+ *   region to be completed and got its result. Note that region_content will be
+ *   empty if the download of that region failed.
+ */
+std::pair<bool, std::string>
+VSICurlFilesystemHandlerBase::NotifyStartDownloadRegion(
     const std::string &osURL, vsi_l_offset startOffset, int nBlocks)
 {
     std::string osId(osURL);
@@ -1688,7 +1706,7 @@ std::string VSICurlFilesystemHandlerBase::NotifyStartDownloadRegion(
         std::string osRet = region.osData;
         region.nWaiters--;
         region.oCond.notify_one();
-        return osRet;
+        return std::pair<bool, std::string>(true, osRet);
     }
     else
     {
@@ -1696,7 +1714,7 @@ std::string VSICurlFilesystemHandlerBase::NotifyStartDownloadRegion(
         poRegionInDownload->bDownloadInProgress = true;
         m_oMapRegionInDownload[osId] = std::move(poRegionInDownload);
         m_oMutex.unlock();
-        return std::string();
+        return std::pair<bool, std::string>(false, std::string());
     }
 }
 
@@ -1752,10 +1770,10 @@ std::string VSICurlHandle::DownloadRegion(const vsi_l_offset startOffset,
     // Check if there is not a download of the same region in progress in
     // another thread, and if so wait for it to be completed
     CurrentDownload currentDownload(poFS, m_pszURL, startOffset, nBlocks);
-    const std::string &osAlreadyDownloadedData =
-        currentDownload.GetAlreadyDownloadedData();
-    if (!osAlreadyDownloadedData.empty())
-        return osAlreadyDownloadedData;
+    if (currentDownload.HasAlreadyDownloadedData())
+    {
+        return currentDownload.GetAlreadyDownloadedData();
+    }
 
 begin:
     CURLM *hCurlMultiHandle = poFS->GetCurlMultiHandleFor(m_pszURL);
diff --git a/port/cpl_vsil_curl_class.h b/port/cpl_vsil_curl_class.h
index 62f8141cfaa2..a2bc96560c74 100644
--- a/port/cpl_vsil_curl_class.h
+++ b/port/cpl_vsil_curl_class.h
@@ -48,6 +48,7 @@
 #include <memory>
 #include <mutex>
 #include <thread>
+#include <utility>
 
 // To avoid aliasing to CopyFile to CopyFileA on Windows
 #ifdef CopyFile
@@ -308,9 +309,9 @@ class VSICurlFilesystemHandlerBase : public VSIFilesystemHandler
     void AddRegion(const char *pszURL, vsi_l_offset nFileOffsetStart,
                    size_t nSize, const char *pData);
 
-    std::string NotifyStartDownloadRegion(const std::string &osURL,
-                                          vsi_l_offset startOffset,
-                                          int nBlocks);
+    std::pair<bool, std::string>
+    NotifyStartDownloadRegion(const std::string &osURL,
+                              vsi_l_offset startOffset, int nBlocks);
     void NotifyStopDownloadRegion(const std::string &osURL,
                                   vsi_l_offset startOffset, int nBlocks,
                                   const std::string &osData);