From 95cfc16bca9accdb6149b73dd61230df3e45b589 Mon Sep 17 00:00:00 2001 From: arvidn Date: Wed, 30 May 2018 14:48:50 +0200 Subject: [PATCH 1/4] fix integer overflow in alert_manager --- Jamfile | 2 +- include/libtorrent/alert_manager.hpp | 8 ++++---- .../aux_/alert_manager_variadic_emplace.hpp | 4 ++-- test/test_alert_manager.cpp | 20 +++++++++++++++++++ 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/Jamfile b/Jamfile index 41090d318..edc1f7dba 100644 --- a/Jamfile +++ b/Jamfile @@ -421,7 +421,7 @@ feature.compose off : TORRENT_USE_IPV6=0 ; feature sanitize : off address undefined thread rtc : composite propagated link-incompatible ; # sanitize is a clang and GCC feature -feature.compose undefined : -fsanitize=undefined -fsanitize=undefined ; +feature.compose undefined : -fsanitize=undefined -fsanitize-undefined-trap-on-error -fsanitize=undefined ; feature.compose thread : -fsanitize=thread -fsanitize=thread ; feature.compose address : -fsanitize=address -fsanitize=address ; # RTC (runtime check) is an msvc feature diff --git a/include/libtorrent/alert_manager.hpp b/include/libtorrent/alert_manager.hpp index beac278fd..f308b8c2c 100644 --- a/include/libtorrent/alert_manager.hpp +++ b/include/libtorrent/alert_manager.hpp @@ -93,8 +93,8 @@ namespace libtorrent { // don't add more than this number of alerts, unless it's a // high priority alert, in which case we try harder to deliver it // for high priority alerts, double the upper limit - if (m_alerts[m_generation].size() >= m_queue_size_limit - * (1 + T::priority)) + if (m_alerts[m_generation].size() / (1 + T::priority) + >= m_queue_size_limit) return; T alert(m_allocations[m_generation], std::forward(args)...); @@ -118,8 +118,8 @@ namespace libtorrent { bool should_post() const { recursive_mutex::scoped_lock lock(m_mutex); - if (m_alerts[m_generation].size() >= m_queue_size_limit - * (1 + T::priority)) + if (m_alerts[m_generation].size() / (1 + T::priority) + >= m_queue_size_limit) { return false; } diff --git a/include/libtorrent/aux_/alert_manager_variadic_emplace.hpp b/include/libtorrent/aux_/alert_manager_variadic_emplace.hpp index 180632edd..3cca6181e 100644 --- a/include/libtorrent/aux_/alert_manager_variadic_emplace.hpp +++ b/include/libtorrent/aux_/alert_manager_variadic_emplace.hpp @@ -37,8 +37,8 @@ // don't add more than this number of alerts, unless it's a // high priority alert, in which case we try harder to deliver it // for high priority alerts, double the upper limit - if (m_alerts[m_generation].size() >= m_queue_size_limit - * (1 + T::priority)) + if (m_alerts[m_generation].size() / (1 + T::priority) + >= m_queue_size_limit) return; T alert(m_allocations[m_generation] diff --git a/test/test_alert_manager.cpp b/test/test_alert_manager.cpp index a1de83797..fbb2c5ecc 100644 --- a/test/test_alert_manager.cpp +++ b/test/test_alert_manager.cpp @@ -81,6 +81,26 @@ TORRENT_TEST(limit) TEST_EQUAL(alerts.size(), 200); } +TORRENT_TEST(limit_int_max) +{ + int const inf = std::numeric_limits::max(); + alert_manager mgr(inf, 0xffffffff); + + TEST_EQUAL(mgr.alert_queue_size_limit(), inf); + + for (int i = 0; i < 600; ++i) + mgr.emplace_alert(torrent_handle(), i); + + for (int i = 0; i < 600; ++i) + mgr.emplace_alert(torrent_handle(), sha1_hash()); + + std::vector alerts; + int num_resume; + mgr.get_all(alerts, num_resume); + + TEST_EQUAL(alerts.size(), 1200); +} + TORRENT_TEST(priority_limit) { alert_manager mgr(100, 0xffffffff); From 794ccf4f793d3e62b2287cba60010014c54389d0 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sun, 3 Jun 2018 21:04:08 +0200 Subject: [PATCH 2/4] remove unused disk stats counters --- include/libtorrent/disk_io_thread.hpp | 12 -------- src/disk_io_thread.cpp | 43 ++++++++++----------------- src/storage.cpp | 2 -- 3 files changed, 15 insertions(+), 42 deletions(-) diff --git a/include/libtorrent/disk_io_thread.hpp b/include/libtorrent/disk_io_thread.hpp index aad057c00..37da2d310 100644 --- a/include/libtorrent/disk_io_thread.hpp +++ b/include/libtorrent/disk_io_thread.hpp @@ -567,18 +567,6 @@ namespace libtorrent counters& m_stats_counters; - // average read time for cache misses (in microseconds) - average_accumulator m_read_time; - - // average write time (in microseconds) - average_accumulator m_write_time; - - // average hash time (in microseconds) - average_accumulator m_hash_time; - - // average time to serve a job (any job) in microseconds - average_accumulator m_job_time; - // this is the main thread io_service. Callbacks are // posted on this in order to have them execute in // the main thread. diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 5a092bc82..3c0865867 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -616,8 +616,8 @@ namespace libtorrent TORRENT_PIECE_ASSERT(num_blocks > 0, pe); m_stats_counters.inc_stats_counter(counters::num_writing_threads, 1); - time_point start_time = clock_type::now(); - int block_size = m_disk_cache.block_size(); + time_point const start_time = clock_type::now(); + int const block_size = m_disk_cache.block_size(); #if DEBUG_DISK_THREAD DLOG("flush_iovec: piece: %d [ ", int(pe->piece)); @@ -653,8 +653,7 @@ namespace libtorrent if (!failed) { TORRENT_PIECE_ASSERT(!error, pe); - boost::uint32_t write_time = total_microseconds(clock_type::now() - start_time); - m_write_time.add_sample(write_time / num_blocks); + boost::uint32_t const write_time = total_microseconds(clock_type::now() - start_time); m_stats_counters.inc_stats_counter(counters::num_blocks_written, num_blocks); m_stats_counters.inc_stats_counter(counters::num_write_ops); @@ -1113,8 +1112,6 @@ namespace libtorrent TORRENT_ASSERT(j->action < sizeof(job_functions)/sizeof(job_functions[0])); - time_point start_time = clock_type::now(); - m_stats_counters.inc_stats_counter(counters::num_running_disk_jobs, 1); // call disk function @@ -1166,8 +1163,6 @@ namespace libtorrent j->ret = ret; - time_point now = clock_type::now(); - m_job_time.add_sample(total_microseconds(now - start_time)); completed_jobs.push_back(j); } @@ -1181,7 +1176,7 @@ namespace libtorrent return -1; } - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); int const file_flags = file_flags_for_job(j , m_settings.get_bool(settings_pack::coalesce_reads)); @@ -1194,8 +1189,7 @@ namespace libtorrent if (!j->error.ec) { - boost::uint32_t read_time = total_microseconds(clock_type::now() - start_time); - m_read_time.add_sample(read_time); + boost::uint32_t const read_time = total_microseconds(clock_type::now() - start_time); m_stats_counters.inc_stats_counter(counters::num_read_back); m_stats_counters.inc_stats_counter(counters::num_blocks_read); @@ -1260,7 +1254,7 @@ namespace libtorrent int const file_flags = file_flags_for_job(j , m_settings.get_bool(settings_pack::coalesce_reads)); - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); ret = j->storage->get_storage_impl()->readv(iov, iov_len , j->piece, adjusted_offset, file_flags, j->error); @@ -1268,7 +1262,6 @@ namespace libtorrent if (!j->error.ec) { boost::uint32_t const read_time = total_microseconds(clock_type::now() - start_time); - m_read_time.add_sample(read_time / iov_len); m_stats_counters.inc_stats_counter(counters::num_blocks_read, iov_len); m_stats_counters.inc_stats_counter(counters::num_read_ops); @@ -1414,7 +1407,7 @@ namespace libtorrent int disk_io_thread::do_uncached_write(disk_io_job* j) { - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); file::iovec_t const b = { j->buffer.disk_block, size_t(j->d.io.buffer_size) }; int const file_flags = file_flags_for_job(j @@ -1430,8 +1423,7 @@ namespace libtorrent if (!j->error.ec) { - boost::uint32_t write_time = total_microseconds(clock_type::now() - start_time); - m_write_time.add_sample(write_time); + boost::uint32_t const write_time = total_microseconds(clock_type::now() - start_time); m_stats_counters.inc_stats_counter(counters::num_blocks_written); m_stats_counters.inc_stats_counter(counters::num_write_ops); @@ -2177,7 +2169,7 @@ namespace libtorrent l.unlock(); - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); for (int i = cursor; i < end; ++i) { @@ -2187,7 +2179,7 @@ namespace libtorrent offset += size; } - boost::uint64_t hash_time = total_microseconds(clock_type::now() - start_time); + boost::uint64_t const hash_time = total_microseconds(clock_type::now() - start_time); l.lock(); @@ -2197,8 +2189,6 @@ namespace libtorrent TORRENT_PIECE_ASSERT(pe->hashing, pe); TORRENT_PIECE_ASSERT(pe->hash, pe); - m_hash_time.add_sample(hash_time / (end - cursor)); - m_stats_counters.inc_stats_counter(counters::num_blocks_hashed, end - cursor); m_stats_counters.inc_stats_counter(counters::disk_hash_time, hash_time); m_stats_counters.inc_stats_counter(counters::disk_job_time, hash_time); @@ -2270,7 +2260,7 @@ namespace libtorrent DLOG("do_hash: (uncached) reading (piece: %d block: %d)\n" , int(j->piece), i); - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); iov.iov_len = (std::min)(block_size, piece_size - offset); ret = j->storage->get_storage_impl()->readv(&iov, 1, j->piece @@ -2280,7 +2270,6 @@ namespace libtorrent if (!j->error.ec) { boost::uint32_t const read_time = total_microseconds(clock_type::now() - start_time); - m_read_time.add_sample(read_time); m_stats_counters.inc_stats_counter(counters::num_blocks_read); m_stats_counters.inc_stats_counter(counters::num_read_ops); @@ -2465,7 +2454,7 @@ namespace libtorrent DLOG("do_hash: reading (piece: %d block: %d)\n", int(pe->piece), i); - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); TORRENT_PIECE_ASSERT(offset == i * block_size, pe); ret = j->storage->get_storage_impl()->readv(&iov, 1, j->piece @@ -2495,8 +2484,7 @@ namespace libtorrent if (!j->error.ec) { - boost::uint32_t read_time = total_microseconds(clock_type::now() - start_time); - m_read_time.add_sample(read_time); + boost::uint32_t const read_time = total_microseconds(clock_type::now() - start_time); m_stats_counters.inc_stats_counter(counters::num_read_back); m_stats_counters.inc_stats_counter(counters::num_blocks_read); @@ -2720,7 +2708,7 @@ namespace libtorrent DLOG("do_cache_piece: reading (piece: %d block: %d)\n" , int(pe->piece), i); - time_point start_time = clock_type::now(); + time_point const start_time = clock_type::now(); ret = j->storage->get_storage_impl()->readv(&iov, 1, j->piece , offset, file_flags, j->error); @@ -2733,8 +2721,7 @@ namespace libtorrent if (!j->error.ec) { - boost::uint32_t read_time = total_microseconds(clock_type::now() - start_time); - m_read_time.add_sample(read_time); + boost::uint32_t const read_time = total_microseconds(clock_type::now() - start_time); m_stats_counters.inc_stats_counter(counters::num_blocks_read); m_stats_counters.inc_stats_counter(counters::num_read_ops); diff --git a/src/storage.cpp b/src/storage.cpp index a2ddef25d..4a4712d15 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -1973,8 +1973,6 @@ namespace libtorrent , static_cast(this), m_has_fence, int(m_outstanding_jobs)); // if this is the job that raised the fence, don't block it - // ignore fence can only ignore one fence. If there are several, - // this job still needs to get queued up if (m_has_fence == 0) { TORRENT_ASSERT((j->flags & disk_io_job::in_progress) == 0); From 24082004758a7510af9d7feffd7545d2ee6ff6c3 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sun, 3 Jun 2018 21:00:59 +0200 Subject: [PATCH 3/4] fix race condition in stat_cache --- ChangeLog | 1 + include/libtorrent/stat_cache.hpp | 4 +++- src/stat_cache.cpp | 8 ++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index d6ec9ef20..0fdd62218 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,5 @@ + * fix race condition in stat_cache (disk storage) * improve error handling of failing to change file priority The API for custom storage implementations was altered * set the hidden attribute when creating the part file diff --git a/include/libtorrent/stat_cache.hpp b/include/libtorrent/stat_cache.hpp index 47b204161..a88474fe9 100644 --- a/include/libtorrent/stat_cache.hpp +++ b/include/libtorrent/stat_cache.hpp @@ -42,6 +42,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/aux_/disable_warnings_pop.hpp" #include "libtorrent/config.hpp" +#include "libtorrent/thread.hpp" namespace libtorrent { @@ -51,7 +52,7 @@ namespace libtorrent ~stat_cache(); void init(int num_files); - + enum { cache_error = -1, @@ -79,6 +80,7 @@ namespace libtorrent boost::int64_t file_size; time_t file_time; }; + mutable mutex m_mutex; std::vector m_stat_cache; }; } diff --git a/src/stat_cache.cpp b/src/stat_cache.cpp index a2ee2d70e..cdd4567cb 100644 --- a/src/stat_cache.cpp +++ b/src/stat_cache.cpp @@ -41,6 +41,7 @@ namespace libtorrent void stat_cache::set_cache(int i, boost::int64_t size, time_t time) { TORRENT_ASSERT(i >= 0); + mutex::scoped_lock l(m_mutex); if (i >= int(m_stat_cache.size())) m_stat_cache.resize(i + 1, not_in_cache); m_stat_cache[i].file_size = size; @@ -50,6 +51,7 @@ namespace libtorrent void stat_cache::set_dirty(int i) { TORRENT_ASSERT(i >= 0); + mutex::scoped_lock l(m_mutex); if (i >= int(m_stat_cache.size())) return; m_stat_cache[i].file_size = not_in_cache; } @@ -57,6 +59,7 @@ namespace libtorrent void stat_cache::set_noexist(int i) { TORRENT_ASSERT(i >= 0); + mutex::scoped_lock l(m_mutex); if (i >= int(m_stat_cache.size())) m_stat_cache.resize(i + 1, not_in_cache); m_stat_cache[i].file_size = no_exist; @@ -65,6 +68,7 @@ namespace libtorrent void stat_cache::set_error(int i) { TORRENT_ASSERT(i >= 0); + mutex::scoped_lock l(m_mutex); if (i >= int(m_stat_cache.size())) m_stat_cache.resize(i + 1, not_in_cache); m_stat_cache[i].file_size = cache_error; @@ -72,12 +76,14 @@ namespace libtorrent boost::int64_t stat_cache::get_filesize(int i) const { + mutex::scoped_lock l(m_mutex); if (i >= int(m_stat_cache.size())) return not_in_cache; return m_stat_cache[i].file_size; } time_t stat_cache::get_filetime(int i) const { + mutex::scoped_lock l(m_mutex); if (i >= int(m_stat_cache.size())) return not_in_cache; if (m_stat_cache[i].file_size < 0) return m_stat_cache[i].file_size; return m_stat_cache[i].file_time; @@ -85,11 +91,13 @@ namespace libtorrent void stat_cache::init(int num_files) { + mutex::scoped_lock l(m_mutex); m_stat_cache.resize(num_files, not_in_cache); } void stat_cache::clear() { + mutex::scoped_lock l(m_mutex); std::vector().swap(m_stat_cache); } From 049d867c129f3c9ee91ad0fd16b5a6fd06e30689 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sun, 3 Jun 2018 19:33:00 +0200 Subject: [PATCH 4/4] fixed race condition in random number generator --- ChangeLog | 1 + src/random.cpp | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0fdd62218..926a3a624 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,5 @@ + * fixed race condition in random number generator * fix race condition in stat_cache (disk storage) * improve error handling of failing to change file priority The API for custom storage implementations was altered diff --git a/src/random.cpp b/src/random.cpp index f5b65d085..f9dc5e79e 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -33,6 +33,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/config.hpp" #include "libtorrent/random.hpp" #include "libtorrent/assert.hpp" +#include "libtorrent/thread.hpp" #ifdef BOOST_NO_CXX11_HDR_RANDOM #include "libtorrent/aux_/disable_warnings_push.hpp" @@ -46,10 +47,6 @@ POSSIBILITY OF SUCH DAMAGE. #include #endif -#if !TORRENT_THREADSAFE_STATIC -#include "libtorrent/thread.hpp" -#endif - namespace libtorrent { #ifdef BOOST_NO_CXX11_HDR_RANDOM @@ -73,25 +70,28 @@ namespace libtorrent #else -#if !TORRENT_THREADSAFE_STATIC - // because local statics are not atomic pre c++11 - // do it manually, probably at a higher cost namespace { - static mutex random_device_mutex; - static random_device* dev = NULL; - static mt19937* rnd = NULL; - } +#if !TORRENT_THREADSAFE_STATIC + // because local statics are not atomic pre c++11 + // do it manually, probably at a higher cost + random_device* dev = NULL; + mt19937* rnd = NULL; #endif + mutex random_device_mutex; + } + boost::uint32_t random() { + // TODO: use a thread local mt19937 instance instead! + mutex::scoped_lock l(random_device_mutex); + #if TORRENT_THREADSAFE_STATIC static random_device dev; static mt19937 random_engine(dev()); return uniform_int_distribution(0, UINT_MAX)(random_engine); #else - mutex::scoped_lock l(random_device_mutex); if (dev == NULL) {