refactor: remove tr_torrent::do_magnet_idle_work() (#7271)

* Revert "fix: possible heap-use-after-free with magnet links (#6815)"

This reverts commit 09b67c84b1.

* fix: check if torrent still exists before verifying

* refactor: queue `on_have_all_metainfo()` in session thread instead

libtransmission/torrent-magnet.cc

@@ -68,15 +68,6 @@ tr_metadata_download::tr_metadata_download(std::string_view log_name, int64_t co
     create_all_needed(n);
 }
 
-void tr_torrent::do_magnet_idle_work()
-{
-    if (auto& m = metadata_download_; m && m->is_complete())
-    {
-        tr_logAddDebugTor(this, fmt::format("we now have all the metainfo!"));
-        on_have_all_metainfo();
-    }
-}
-
 void tr_torrent::maybe_start_metadata_transfer(int64_t const size) noexcept
 {
     if (has_metainfo() || metadata_download_)
@@ -258,7 +249,10 @@ tr_variant build_metainfo_except_info_dict(tr_torrent_metainfo const& tm)
 void tr_torrent::on_have_all_metainfo()
 {
     auto& m = metadata_download_;
-    TR_ASSERT(m);
+    if (!m)
+    {
+        return;
+    }
 
     if (auto error = tr_error{}; !use_new_metainfo(&error)) /* drat. */
     {
@@ -271,20 +265,20 @@ void tr_torrent::on_have_all_metainfo()
     m.reset();
 }
 
-void tr_metadata_download::set_metadata_piece(int64_t const piece, void const* const data, size_t const len)
+bool tr_metadata_download::set_metadata_piece(int64_t const piece, void const* const data, size_t const len)
 {
     TR_ASSERT(data != nullptr);
 
     // sanity test: is `piece` in range?
     if (piece < 0 || piece >= piece_count_)
     {
-        return;
+        return false;
     }
 
     // sanity test: is `len` the right size?
     if (get_piece_length(piece) != len)
     {
-        return;
+        return false;
     }
 
     // do we need this piece?
@@ -295,7 +289,7 @@ void tr_metadata_download::set_metadata_piece(int64_t const piece, void const* c
         [piece](auto const& item) { return item.piece == piece; });
     if (iter == std::end(needed))
     {
-        return;
+        return false;
     }
 
     auto const offset = piece * MetadataPieceSize;
@@ -303,6 +297,8 @@ void tr_metadata_download::set_metadata_piece(int64_t const piece, void const* c
 
     needed.erase(iter);
     tr_logAddDebugMagnet(this, fmt::format("saving metainfo piece {}... {} remain", piece, std::size(needed)));
+
+    return std::empty(needed);
 }
 
 void tr_torrent::set_metadata_piece(int64_t const piece, void const* const data, size_t const len)
@@ -311,9 +307,20 @@ void tr_torrent::set_metadata_piece(int64_t const piece, void const* const data,
 
     tr_logAddDebugTor(this, fmt::format("got metadata piece {} of {} bytes", piece, len));
 
-    if (auto& m = metadata_download_)
+    if (auto& m = metadata_download_; m && m->set_metadata_piece(piece, data, len))
     {
-        m->set_metadata_piece(piece, data, len);
+        tr_logAddDebugTor(this, fmt::format("we now have all the metainfo!"));
+
+        // Why queue this invocation in session thread:
+        // https://github.com/transmission/transmission/pull/6383#discussion_r1429202253
+        session->queue_session_thread(
+            [s = session, id = id()]
+            {
+                if (auto* tor = s->torrents().get(id); tor != nullptr)
+                {
+                    tor->on_have_all_metainfo();
+                }
+            });
     }
 }
 

libtransmission/torrent-magnet.h

@@ -38,12 +38,7 @@ public:
         return size > 0 && size <= std::numeric_limits<int>::max();
     }
 
-    [[nodiscard]] auto is_complete() const noexcept
+    bool set_metadata_piece(int64_t piece, void const* data, size_t len);
-    {
-        return std::empty(pieces_needed_);
-    }
-
-    void set_metadata_piece(int64_t piece, void const* data, size_t len);
 
     [[nodiscard]] std::optional<int64_t> get_next_metadata_request(time_t now) noexcept;
 

libtransmission/torrent.cc

@@ -1601,8 +1601,14 @@ void tr_torrentStartNow(tr_torrent* tor)
 void tr_torrentVerify(tr_torrent* tor)
 {
     tor->session->run_in_session_thread(
-        [tor]()
+        [session = tor->session, tor_id = tor->id()]()
         {
+            auto* const tor = session->torrents().get(tor_id);
+            if (tor == nullptr)
+            {
+                return;
+            }
+
             TR_ASSERT(tor->session->am_in_session_thread());
             auto const lock = tor->unique_lock();
 

libtransmission/torrent.h

@@ -898,8 +898,6 @@ struct tr_torrent
 
     void do_idle_work()
     {
-        do_magnet_idle_work();
-
         if (needs_completeness_check_)
         {
             needs_completeness_check_ = false;
@@ -1270,7 +1268,6 @@ private:
     void create_empty_files() const;
     void recheck_completeness();
 
-    void do_magnet_idle_work();
     [[nodiscard]] bool use_new_metainfo(tr_error* error);
 
     void update_file_path(tr_file_index_t file, std::optional<bool> has_file) const;

tests/libtransmission/torrent-magnet-test.cc

@@ -74,25 +74,19 @@ TEST_F(TorrentMagnetTest, setMetadataPiece)
     EXPECT_NE(nullptr, tor);
     EXPECT_FALSE(tor->has_metainfo());
 
-    auto promise = std::promise<void>{};
-    auto future = promise.get_future();
-    session_->run_in_session_thread(
-        [tor, &promise]()
-        {
     auto const metainfo_benc = tr_base64_decode(InfoDictBase64);
     auto const metainfo_size = std::size(metainfo_benc);
     EXPECT_LE(metainfo_size, MetadataPieceSize);
-
+    session_->run_in_session_thread(
+        [&]()
+        {
             tor->maybe_start_metadata_transfer(metainfo_size);
             tor->set_metadata_piece(0, std::data(metainfo_benc), metainfo_size);
-            tor->do_idle_work();
+        });
-            EXPECT_TRUE(tor->has_metainfo());
+
+    EXPECT_TRUE(waitFor([tor] { return tor->has_metainfo(); }, 5s));
     EXPECT_EQ(tor->info_dict_size(), metainfo_size);
     EXPECT_EQ(tor->get_metadata_percent(), 1.0);
-
-            promise.set_value();
-        });
-    future.wait();
 }
 
 } // namespace libtransmission::test