From 3ede0b9c206c47ff5f4e1bc215ca7b121218a90b Mon Sep 17 00:00:00 2001 From: arvidn Date: Fri, 20 Apr 2018 13:03:36 +0200 Subject: [PATCH 1/5] fix last_upload, last_download and last_scrape to be reported accurately and saved/restored in resume data --- docs/manual.rst | 9 ++++++ include/libtorrent/torrent.hpp | 21 +++++-------- src/torrent.cpp | 55 +++++++++++++++++----------------- test/test_resume.cpp | 19 ++++++------ 4 files changed, 54 insertions(+), 50 deletions(-) diff --git a/docs/manual.rst b/docs/manual.rst index 2efb6b680..858bb14c8 100644 --- a/docs/manual.rst +++ b/docs/manual.rst @@ -459,6 +459,15 @@ The file format is a bencoded dictionary containing the following fields: | | torrent when the resume data was last saved. This is used as | | | an initial estimate until we acquire up-to-date scrape info. | +--------------------------+--------------------------------------------------------------+ +| ``last_upload`` | integer. The number of seconds since epoch when we last | +| | uploaded payload to a peer on this torrent. | ++--------------------------+--------------------------------------------------------------+ +| ``last_download`` | integer. The number of seconds since epoch when we last | +| | downloaded payload from a peer on this torrent. | ++--------------------------+--------------------------------------------------------------+ +| ``last_scrape`` | integer. The number of seconds since epoch when we last sent | +| | a scrape request to a tracker on this torrent. | ++--------------------------+--------------------------------------------------------------+ | ``upload_rate_limit`` | integer. In case this torrent has a per-torrent upload rate | | | limit, this is that limit. In bytes per second. | +--------------------------+--------------------------------------------------------------+ diff --git a/include/libtorrent/torrent.hpp b/include/libtorrent/torrent.hpp index f165b10be..44946ff72 100644 --- a/include/libtorrent/torrent.hpp +++ b/include/libtorrent/torrent.hpp @@ -735,11 +735,7 @@ namespace libtorrent void scrape_tracker(int idx, bool user_triggered); void announce_with_tracker(boost::uint8_t e = tracker_request::none); - int seconds_since_last_scrape() const - { - return m_last_scrape == (std::numeric_limits::min)() - ? -1 : int(m_ses.session_time() - m_last_scrape); - } + int seconds_since_last_scrape() const; #ifndef TORRENT_DISABLE_DHT void dht_announce(); @@ -1089,7 +1085,8 @@ namespace libtorrent // that are not private void lsd_announce(); - void update_last_upload() { m_last_upload = m_ses.session_time(); } + void update_last_upload() + { m_last_upload = total_seconds(clock_type::now().time_since_epoch()); } void set_apply_ip_filter(bool b); bool apply_ip_filter() const { return m_apply_ip_filter; } @@ -1654,9 +1651,8 @@ namespace libtorrent // ---- // the timestamp of the last piece passed for this torrent specified in - // session_time. This is signed because it must be able to represent time - // before the session started - boost::int16_t m_last_download; + // seconds since epoch. + boost::uint32_t m_last_download; // the number of peer connections to seeds. This should be the same as // counting the peer connections that say true for is_seed() @@ -1667,9 +1663,8 @@ namespace libtorrent boost::uint16_t m_num_connecting_seeds; // the timestamp of the last byte uploaded from this torrent specified in - // session_time. This is signed because it must be able to represent time - // before the session started. - boost::int16_t m_last_upload; + // seconds since epoch. + boost::uint32_t m_last_upload; // this is a second count-down to when we should tick the // storage for this torrent. Ticking the storage is used @@ -1709,7 +1704,7 @@ namespace libtorrent // the timestamp of the last scrape request to one of the trackers in // this torrent specified in session_time. This is signed because it must // be able to represent time before the session started - boost::int16_t m_last_scrape; + boost::uint32_t m_last_scrape; // ---- diff --git a/src/torrent.cpp b/src/torrent.cpp index 0e5f90633..075bbeb52 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -115,6 +115,8 @@ namespace libtorrent { namespace { + boost::uint32_t const unset = std::numeric_limits::max(); + bool is_downloading_state(int st) { switch (st) @@ -302,17 +304,17 @@ namespace libtorrent , m_deleted(false) , m_pinned((p.flags & add_torrent_params::flag_pinned) != 0) , m_should_be_loaded(true) - , m_last_download((std::numeric_limits::min)()) + , m_last_download(unset) , m_num_seeds(0) , m_num_connecting_seeds(0) - , m_last_upload((std::numeric_limits::min)()) + , m_last_upload(unset) , m_storage_tick(0) , m_auto_managed(p.flags & add_torrent_params::flag_auto_managed) , m_current_gauge_state(no_gauge_state) , m_moving_storage(false) , m_inactive(false) , m_downloaded(0xffffff) - , m_last_scrape((std::numeric_limits::min)()) + , m_last_scrape(unset) , m_progress_ppm(0) , m_pending_active_change(false) , m_use_resume_save_path((p.flags & add_torrent_params::flag_use_resume_save_path) != 0) @@ -3393,10 +3395,16 @@ namespace { update_tracker_timer(now); } + int torrent::seconds_since_last_scrape() const + { + return m_last_scrape == unset ? -1 + : total_seconds(clock_type::now() - time_point(seconds(m_last_scrape))); + } + void torrent::scrape_tracker(int idx, bool user_triggered) { TORRENT_ASSERT(is_single_thread()); - m_last_scrape = m_ses.session_time(); + m_last_scrape = total_seconds(clock_type::now().time_since_epoch()); if (m_trackers.empty()) return; @@ -3555,7 +3563,7 @@ namespace { update_tracker_timer(now); if (resp.complete >= 0 && resp.incomplete >= 0) - m_last_scrape = m_ses.session_time(); + m_last_scrape = total_seconds(clock_type::now().time_since_epoch()); #ifndef TORRENT_DISABLE_LOGGING std::string resolved_to; @@ -4403,7 +4411,7 @@ namespace { // is deallocated by the torrent once it starts seeding } - m_last_download = m_ses.session_time(); + m_last_download = total_seconds(clock_type::now().time_since_epoch()); if (m_share_mode) recalc_share_mode(); @@ -7027,13 +7035,12 @@ namespace { #endif } - int now = m_ses.session_time(); - int tmp = rd.dict_find_int_value("last_scrape", -1); - m_last_scrape = tmp == -1 ? (std::numeric_limits::min)() : now - tmp; + boost::int64_t tmp = rd.dict_find_int_value("last_scrape", -1); + m_last_scrape = tmp == -1 ? unset : tmp; tmp = rd.dict_find_int_value("last_download", -1); - m_last_download = tmp == -1 ? (std::numeric_limits::min)() : now - tmp; + m_last_download = tmp == -1 ? unset : tmp; tmp = rd.dict_find_int_value("last_upload", -1); - m_last_upload = tmp == -1 ? (std::numeric_limits::min)() : now - tmp; + m_last_upload = tmp == -1 ? unset : tmp; if (m_use_resume_save_path) { @@ -7275,6 +7282,9 @@ namespace { ret["num_complete"] = m_complete; ret["num_incomplete"] = m_incomplete; ret["num_downloaded"] = m_downloaded; + ret["last_upload"] = m_last_upload == unset ? -1 : boost::int64_t(m_last_upload); + ret["last_download"] = m_last_download == unset ? -1 : boost::int64_t(m_last_download); + ret["last_scrape"] = m_last_scrape == unset ? -1 : boost::int64_t(m_last_scrape); ret["sequential_download"] = m_sequential_download; @@ -9613,13 +9623,6 @@ namespace { return a - b; } - int clamped_subtract_s16(int a, int b) - { - if (a + (std::numeric_limits::min)() < b) - return (std::numeric_limits::min)(); - return a - b; - } - } // anonymous namespace // this is called every time the session timer takes a step back. Since the @@ -9674,10 +9677,6 @@ namespace { } m_became_finished = clamped_subtract(m_became_finished, seconds); - m_last_upload = clamped_subtract_s16(m_last_upload, seconds); - m_last_download = clamped_subtract_s16(m_last_download, seconds); - m_last_scrape = clamped_subtract_s16(m_last_scrape, seconds); - m_last_saved_resume = clamped_subtract(m_last_saved_resume, seconds); m_upload_mode_time = clamped_subtract(m_upload_mode_time, seconds); } @@ -12145,8 +12144,8 @@ namespace { st->added_time = m_added_time; st->completed_time = m_completed_time; - st->last_scrape = m_last_scrape == (std::numeric_limits::min)() ? -1 - : clamped_subtract(m_ses.session_time(), m_last_scrape); + st->last_scrape = m_last_scrape == unset ? -1 + : total_seconds(clock_type::now() - time_point(seconds(m_last_scrape))); st->share_mode = m_share_mode; st->upload_mode = m_upload_mode; @@ -12168,10 +12167,10 @@ namespace { st->finished_time = finished_time(); st->active_time = active_time(); st->seeding_time = seeding_time(); - st->time_since_upload = m_last_upload == (std::numeric_limits::min)() ? -1 - : clamped_subtract(m_ses.session_time(), m_last_upload); - st->time_since_download = m_last_download == (std::numeric_limits::min)() ? -1 - : clamped_subtract(m_ses.session_time(), m_last_download); + st->time_since_upload = m_last_upload == unset ? -1 + : total_seconds(clock_type::now() - time_point(seconds(m_last_upload))); + st->time_since_download = m_last_download == unset ? -1 + : total_seconds(clock_type::now() - time_point(seconds(m_last_download))); st->storage_mode = static_cast(m_storage_mode); diff --git a/test/test_resume.cpp b/test/test_resume.cpp index 62841ed70..639a9ed8b 100644 --- a/test/test_resume.cpp +++ b/test/test_resume.cpp @@ -99,9 +99,9 @@ std::vector generate_resume_data(torrent_info* ti rd["super_seeding"] = 0; rd["added_time"] = 1347; rd["completed_time"] = 1348; - rd["last_scrape"] = 1349; - rd["last_download"] = 1350; - rd["last_upload"] = 1351; + rd["last_scrape"] = 1; + rd["last_download"] = 2; + rd["last_upload"] = 3; rd["finished_time"] = 1352; if (file_priorities && file_priorities[0]) { @@ -177,14 +177,15 @@ void default_tests(torrent_status const& s) // allow some slack in the time stamps since they are reported as // relative times. If the computer is busy while running the unit test // or running under valgrind it may take several seconds - TEST_CHECK(s.last_scrape >= 1349); - TEST_CHECK(s.time_since_download >= 1350); - TEST_CHECK(s.time_since_upload >= 1351); + int const now = duration_cast(clock_type::now().time_since_epoch()).count(); + TEST_CHECK(s.last_scrape >= now - 1); + TEST_CHECK(s.time_since_download >= now - 2); + TEST_CHECK(s.time_since_upload >= now - 3); TEST_CHECK(s.active_time >= 1339); - TEST_CHECK(s.last_scrape < 1349 + 10); - TEST_CHECK(s.time_since_download < 1350 + 10); - TEST_CHECK(s.time_since_upload < 1351 + 10); + TEST_CHECK(s.last_scrape < now - 1 + 10); + TEST_CHECK(s.time_since_download < now - 2 + 10); + TEST_CHECK(s.time_since_upload < now - 3 + 10); TEST_CHECK(s.active_time < 1339 + 10); TEST_CHECK(s.finished_time >= 1352); From 21d8e9e26b377137612c323992d216be50a547bb Mon Sep 17 00:00:00 2001 From: Xiyue Deng Date: Mon, 23 Apr 2018 02:17:47 -0700 Subject: [PATCH 2/5] Add missing header for va_list. * Fix building on OpenBSD. --- include/libtorrent/aux_/session_interface.hpp | 1 + src/disk_io_thread.cpp | 1 + src/session_impl.cpp | 2 ++ 3 files changed, 4 insertions(+) diff --git a/include/libtorrent/aux_/session_interface.hpp b/include/libtorrent/aux_/session_interface.hpp index b3738bd7c..342e64907 100644 --- a/include/libtorrent/aux_/session_interface.hpp +++ b/include/libtorrent/aux_/session_interface.hpp @@ -52,6 +52,7 @@ POSSIBILITY OF SUCH DAMAGE. #ifndef TORRENT_DISABLE_LOGGING #include +#include // for va_list #endif #ifdef TORRENT_USE_OPENSSL diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 976997bb4..2d4bbd704 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -62,6 +62,7 @@ POSSIBILITY OF SUCH DAMAGE. #if __cplusplus >= 201103L || defined __clang__ #if DEBUG_DISK_THREAD +#include // for va_list #define DLOG(...) debug_log(__VA_ARGS__) #else #define DLOG(...) do {} while(false) diff --git a/src/session_impl.cpp b/src/session_impl.cpp index 1a183500b..b54a5eb18 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -107,6 +107,8 @@ POSSIBILITY OF SUCH DAMAGE. // for logging stat layout #include "libtorrent/stat.hpp" +#include // for va_list + // for logging the size of DHT structures #ifndef TORRENT_DISABLE_DHT #include From 6394e7ac02c28e72afb2f58a2641c6fd73d0b6a0 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Mon, 23 Apr 2018 13:54:22 +0800 Subject: [PATCH 3/5] Fix MSVC warning C4267 by casting to the correct type explictly --- include/libtorrent/bencode.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/libtorrent/bencode.hpp b/include/libtorrent/bencode.hpp index 27c4852df..15b0915ce 100644 --- a/include/libtorrent/bencode.hpp +++ b/include/libtorrent/bencode.hpp @@ -212,7 +212,7 @@ namespace libtorrent break; case entry::preformatted_t: std::copy(e.preformatted().begin(), e.preformatted().end(), out); - ret += e.preformatted().size(); + ret += static_cast(e.preformatted().size()); break; case entry::undefined_t: From 366b7983d1a9181c2ea74240f7a7a5976cea2e5c Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Wed, 25 Apr 2018 23:09:25 -0400 Subject: [PATCH 4/5] fix typo with bind_outgoing_socket~ --- src/peer_connection.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index 44ae840a1..8d6601278 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -381,12 +381,14 @@ namespace libtorrent return; } -#ifndef TORRENT_DISABLE_LOGGING tcp::endpoint const bound_ip = m_ses.bind_outgoing_socket(*m_socket , m_remote.address(), ec); +#ifndef TORRENT_DISABLE_LOGGING peer_log(peer_log_alert::outgoing, "BIND", "dst: %s ec: %s" , print_endpoint(bound_ip).c_str() , ec.message().c_str()); +#else + TORRENT_UNUSED(bound_ip); #endif if (ec) { From f5e33932d2cc85be27d6fdeefd7cb3fb37a73742 Mon Sep 17 00:00:00 2001 From: Steven Siloti Date: Thu, 26 Apr 2018 10:32:02 -0700 Subject: [PATCH 5/5] fix use after free in flush_range and flush_iovec Calling blocks_flushed can cause the piece entry to be freed so its callers need to be aware of that and avoid dereferencing the pointer if the entry is freed. --- include/libtorrent/block_cache.hpp | 3 ++- include/libtorrent/disk_io_thread.hpp | 3 ++- src/block_cache.cpp | 4 ++-- src/disk_io_thread.cpp | 12 +++++++----- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/include/libtorrent/block_cache.hpp b/include/libtorrent/block_cache.hpp index 45fce7a5c..6e0d77631 100644 --- a/include/libtorrent/block_cache.hpp +++ b/include/libtorrent/block_cache.hpp @@ -440,7 +440,8 @@ namespace libtorrent // used to convert dirty blocks into non-dirty ones // i.e. from being part of the write cache to being part // of the read cache. it's used when flushing blocks to disk - void blocks_flushed(cached_piece_entry* pe, int const* flushed, int num_flushed); + // returns true if the piece entry was freed + bool blocks_flushed(cached_piece_entry* pe, int const* flushed, int num_flushed); // adds a block to the cache, marks it as dirty and // associates the job with it. When the block is diff --git a/include/libtorrent/disk_io_thread.hpp b/include/libtorrent/disk_io_thread.hpp index 4000924d2..aad057c00 100644 --- a/include/libtorrent/disk_io_thread.hpp +++ b/include/libtorrent/disk_io_thread.hpp @@ -477,7 +477,8 @@ namespace libtorrent , file::iovec_t* iov, int* flushing, int block_base_index = 0); void flush_iovec(cached_piece_entry* pe, file::iovec_t const* iov, int const* flushing , int num_blocks, storage_error& error); - void iovec_flushed(cached_piece_entry* pe + // returns true if the piece entry was freed + bool iovec_flushed(cached_piece_entry* pe , int* flushing, int num_blocks, int block_offset , storage_error const& error , jobqueue_t& completed_jobs); diff --git a/src/block_cache.cpp b/src/block_cache.cpp index 26b76e2d9..59ba2a1a8 100644 --- a/src/block_cache.cpp +++ b/src/block_cache.cpp @@ -793,7 +793,7 @@ cached_piece_entry* block_cache::add_dirty_block(disk_io_job* j) // (since these blocks now are part of the read cache) the refcounts of the // blocks are also decremented by this function. They are expected to have been // incremented by the caller. -void block_cache::blocks_flushed(cached_piece_entry* pe, int const* flushed, int num_flushed) +bool block_cache::blocks_flushed(cached_piece_entry* pe, int const* flushed, int num_flushed) { TORRENT_PIECE_ASSERT(pe->in_use, pe); @@ -817,7 +817,7 @@ void block_cache::blocks_flushed(cached_piece_entry* pe, int const* flushed, int pe->num_dirty -= num_flushed; update_cache_state(pe); - maybe_free_piece(pe); + return maybe_free_piece(pe); } std::pair block_cache::all_pieces() const diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 2d4bbd704..a6189c26f 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -676,7 +676,7 @@ namespace libtorrent // It is necessary to call this function with the blocks produced by // build_iovec, to reset their state to not being flushed anymore // the cache needs to be locked when calling this function - void disk_io_thread::iovec_flushed(cached_piece_entry* pe + bool disk_io_thread::iovec_flushed(cached_piece_entry* pe , int* flushing, int num_blocks, int block_offset , storage_error const& error , jobqueue_t& completed_jobs) @@ -691,7 +691,8 @@ namespace libtorrent DLOG("%d ", flushing[i]); DLOG("]\n"); #endif - m_disk_cache.blocks_flushed(pe, flushing, num_blocks); + if (m_disk_cache.blocks_flushed(pe, flushing, num_blocks)) + return true; int block_size = m_disk_cache.block_size(); @@ -721,6 +722,8 @@ namespace libtorrent j = next; } } + + return false; } // issues write operations for blocks in the given @@ -756,9 +759,8 @@ namespace libtorrent TORRENT_PIECE_ASSERT(pe->piece_refcount > 0, pe); --pe->piece_refcount; - iovec_flushed(pe, flushing, iov_len, 0, error, completed_jobs); - - m_disk_cache.maybe_free_piece(pe); + if (!iovec_flushed(pe, flushing, iov_len, 0, error, completed_jobs)) + m_disk_cache.maybe_free_piece(pe); // if the cache is under high pressure, we need to evict // the blocks we just flushed to make room for more write pieces