From 09b67c84b1f17ed22b7cce0731867e0029c62fbe Mon Sep 17 00:00:00 2001 From: Yat Ho Date: Sat, 25 May 2024 04:50:01 +0800 Subject: [PATCH] fix: possible heap-use-after-free with magnet links (#6815) * fix: queue torrent verification as soon as metadata complete * fix: avoid heap-use-after-free in `tr_peerMsgsImpl::process_peer_message()` Details: https://github.com/transmission/transmission/pull/6383#discussion_r1429202253 * code review: move `tr_torrent::do_magnet_idle_work()` to private * code review: `std::deque::empty()` is not `constexpr` * fix: test --- libtransmission/torrent-magnet.cc | 24 ++++++++++++-------- libtransmission/torrent-magnet.h | 7 +++++- libtransmission/torrent.cc | 2 +- libtransmission/torrent.h | 3 +++ tests/libtransmission/torrent-magnet-test.cc | 1 + 5 files changed, 26 insertions(+), 11 deletions(-) diff --git a/libtransmission/torrent-magnet.cc b/libtransmission/torrent-magnet.cc index 8732c0745..aa6952676 100644 --- a/libtransmission/torrent-magnet.cc +++ b/libtransmission/torrent-magnet.cc @@ -68,6 +68,15 @@ 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_) @@ -262,20 +271,20 @@ void tr_torrent::on_have_all_metainfo() m.reset(); } -bool tr_metadata_download::set_metadata_piece(int64_t const piece, void const* const data, size_t const len) +void 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 false; + return; } // sanity test: is `len` the right size? if (get_piece_length(piece) != len) { - return false; + return; } // do we need this piece? @@ -286,7 +295,7 @@ bool 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 false; + return; } auto const offset = piece * MetadataPieceSize; @@ -294,8 +303,6 @@ bool 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) @@ -304,10 +311,9 @@ 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_; m && m->set_metadata_piece(piece, data, len)) + if (auto& m = metadata_download_) { - tr_logAddDebugTor(this, fmt::format("we now have all the metainfo!")); - on_have_all_metainfo(); + m->set_metadata_piece(piece, data, len); } } diff --git a/libtransmission/torrent-magnet.h b/libtransmission/torrent-magnet.h index 72fabfb17..dbe6dac3a 100644 --- a/libtransmission/torrent-magnet.h +++ b/libtransmission/torrent-magnet.h @@ -39,7 +39,12 @@ public: return size > 0 && size <= std::numeric_limits::max(); } - bool set_metadata_piece(int64_t piece, void const* data, size_t len); + [[nodiscard]] auto is_complete() const noexcept + { + return std::empty(pieces_needed_); + } + + void set_metadata_piece(int64_t piece, void const* data, size_t len); [[nodiscard]] std::optional get_next_metadata_request(time_t now) noexcept; diff --git a/libtransmission/torrent.cc b/libtransmission/torrent.cc index 49ded80ab..d2e927de1 100644 --- a/libtransmission/torrent.cc +++ b/libtransmission/torrent.cc @@ -899,7 +899,7 @@ void tr_torrent::on_metainfo_completed() // Potentially, we are in `tr_torrent::init`, // and we don't want any file created before `tr_torrent::start` // so we Verify but we don't Create files. - session->queue_session_thread(tr_torrentVerify, this); + tr_torrentVerify(this); } else { diff --git a/libtransmission/torrent.h b/libtransmission/torrent.h index 3b72bffd7..f7ef53e53 100644 --- a/libtransmission/torrent.h +++ b/libtransmission/torrent.h @@ -887,6 +887,8 @@ struct tr_torrent void do_idle_work() { + do_magnet_idle_work(); + if (needs_completeness_check_) { needs_completeness_check_ = false; @@ -1244,6 +1246,7 @@ private: void create_empty_files() const; void recheck_completeness(); + void do_magnet_idle_work(); [[nodiscard]] bool use_new_metainfo(tr_error* error); void set_location_in_session_thread(std::string_view path, bool move_from_old_path, int volatile* setme_state); diff --git a/tests/libtransmission/torrent-magnet-test.cc b/tests/libtransmission/torrent-magnet-test.cc index 3a284f2d8..4cf732e31 100644 --- a/tests/libtransmission/torrent-magnet-test.cc +++ b/tests/libtransmission/torrent-magnet-test.cc @@ -85,6 +85,7 @@ TEST_F(TorrentMagnetTest, setMetadataPiece) 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_EQ(tor->info_dict_size(), metainfo_size); EXPECT_EQ(tor->get_metadata_percent(), 1.0);