fix: clear verify_thread before releasing verify mutex lock (#2360)

* fix: clear verify_thread before releasing verify mutex lock

* refactor: increase the verify buffer size

* fix: uniqueness comparison for verify torrents

libtransmission/verify.cc

@@ -7,10 +7,10 @@
  */
 
 #include <algorithm>
-#include <cstdlib> /* free() */
 #include <ctime>
 #include <mutex>
 #include <set>
+#include <vector>
 
 #include "transmission.h"
 #include "completion.h"
@@ -31,6 +31,8 @@ static auto constexpr MsecToSleepPerSecondDuringVerify = int{ 100 };
 
 static bool verifyTorrent(tr_torrent* tor, bool* stopFlag)
 {
+    auto const begin = tr_time();
+
     tr_sys_file_t fd = TR_BAD_SYS_FILE;
     uint64_t filePos = 0;
     bool changed = false;
@@ -40,11 +42,8 @@ static bool verifyTorrent(tr_torrent* tor, bool* stopFlag)
     tr_file_index_t fileIndex = 0;
     tr_file_index_t prevFileIndex = !fileIndex;
     tr_piece_index_t piece = 0;
-    time_t const begin = tr_time();
+    auto buffer = std::vector<std::byte>(1024 * 256);
-    size_t const buflen = 1024 * 128; // 128 KiB buffer
+    auto sha = tr_sha1_init();
-    auto* const buffer = static_cast<uint8_t*>(tr_malloc(buflen));
-
-    tr_sha1_ctx_t sha = tr_sha1_init();
 
     tr_logAddTorDbg(tor, "%s", "verifying torrent...");
     tor->verify_progress = 0;
@@ -73,16 +72,16 @@ static bool verifyTorrent(tr_torrent* tor, bool* stopFlag)
         uint64_t leftInPiece = tor->pieceSize(piece) - piecePos;
         uint64_t leftInFile = file_length - filePos;
         uint64_t bytesThisPass = std::min(leftInFile, leftInPiece);
-        bytesThisPass = std::min(bytesThisPass, uint64_t{ buflen });
+        bytesThisPass = std::min(bytesThisPass, uint64_t(std::size(buffer)));
 
         /* read a bit */
         if (fd != TR_BAD_SYS_FILE)
         {
             auto numRead = uint64_t{};
-            if (tr_sys_file_read_at(fd, buffer, bytesThisPass, filePos, &numRead, nullptr) && numRead > 0)
+            if (tr_sys_file_read_at(fd, std::data(buffer), bytesThisPass, filePos, &numRead, nullptr) && numRead > 0)
             {
                 bytesThisPass = numRead;
-                tr_sha1_update(sha, buffer, bytesThisPass);
+                tr_sha1_update(sha, std::data(buffer), bytesThisPass);
                 tr_sys_file_advise(fd, filePos, bytesThisPass, TR_SYS_FILE_ADVICE_DONT_NEED, nullptr);
             }
         }
@@ -143,7 +142,6 @@ static bool verifyTorrent(tr_torrent* tor, bool* stopFlag)
 
     tor->verify_progress.reset();
     tr_sha1_final(sha);
-    free(buffer);
 
     /* stopwatch */
     time_t const end = tr_time();
@@ -184,6 +182,12 @@ struct verify_node
             return current_size < that.current_size ? -1 : 1;
         }
 
+        // tertiary compare just to ensure they don't compare equal
+        if (torrent->infoHash() != that.torrent->infoHash())
+        {
+            return torrent->infoHash() < that.torrent->infoHash() ? -1 : 1;
+        }
+
         return 0;
     }
 
@@ -195,8 +199,8 @@ struct verify_node
 
 static struct verify_node currentNode;
 // TODO: refactor s.t. this doesn't leak
-static auto& verifyList{ *new std::set<verify_node>{} };
+static auto& verify_list{ *new std::set<verify_node>{} };
-static tr_thread* verifyThread = nullptr;
+static tr_thread* verify_thread = nullptr;
 static bool stopCurrent = false;
 
 static std::mutex verify_mutex_;
@@ -205,27 +209,26 @@ static void verifyThreadFunc(void* /*user_data*/)
 {
     for (;;)
     {
-        bool changed = false;
-
         {
             auto const lock = std::lock_guard(verify_mutex_);
 
             stopCurrent = false;
-            if (std::empty(verifyList))
+            if (std::empty(verify_list))
             {
                 currentNode.torrent = nullptr;
-                break;
+                verify_thread = nullptr;
+                return;
             }
 
-            auto const it = std::begin(verifyList);
+            auto const it = std::begin(verify_list);
             currentNode = *it;
-            verifyList.erase(it);
+            verify_list.erase(it);
         }
 
         tr_torrent* tor = currentNode.torrent;
         tr_logAddTorInfo(tor, "%s", _("Verifying torrent"));
         tor->setVerifyState(TR_VERIFY_NOW);
-        changed = verifyTorrent(tor, &stopCurrent);
+        auto const changed = verifyTorrent(tor, &stopCurrent);
         tor->setVerifyState(TR_VERIFY_NONE);
         TR_ASSERT(tr_isTorrent(tor));
 
@@ -239,8 +242,6 @@ static void verifyThreadFunc(void* /*user_data*/)
             (*currentNode.callback_func)(tor, stopCurrent, currentNode.callback_data);
         }
     }
-
-    verifyThread = nullptr;
 }
 
 void tr_verifyAdd(tr_torrent* tor, tr_verify_done_func callback_func, void* callback_data)
@@ -256,11 +257,11 @@ void tr_verifyAdd(tr_torrent* tor, tr_verify_done_func callback_func, void* call
 
     auto const lock = std::lock_guard(verify_mutex_);
     tor->setVerifyState(TR_VERIFY_WAIT);
-    verifyList.insert(node);
+    verify_list.insert(node);
 
-    if (verifyThread == nullptr)
+    if (verify_thread == nullptr)
     {
-        verifyThread = tr_threadNew(verifyThreadFunc, nullptr);
+        verify_thread = tr_threadNew(verifyThreadFunc, nullptr);
     }
 }
 
@@ -284,20 +285,20 @@ void tr_verifyRemove(tr_torrent* tor)
     else
     {
         auto const it = std::find_if(
-            std::begin(verifyList),
+            std::begin(verify_list),
-            std::end(verifyList),
+            std::end(verify_list),
             [tor](auto const& task) { return tor == task.torrent; });
 
         tor->setVerifyState(TR_VERIFY_NONE);
 
-        if (it != std::end(verifyList))
+        if (it != std::end(verify_list))
         {
             if (it->callback_func != nullptr)
             {
                 (*it->callback_func)(tor, true, it->callback_data);
             }
 
-            verifyList.erase(it);
+            verify_list.erase(it);
         }
     }
 
@@ -309,5 +310,5 @@ void tr_verifyClose(tr_session* /*session*/)
     auto const lock = std::lock_guard(verify_mutex_);
 
     stopCurrent = true;
-    verifyList.clear();
+    verify_list.clear();
 }