From a4267d61e7b4acc566f459b933056755586f88d6 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Tue, 2 Oct 2018 14:42:40 +0200 Subject: [PATCH] fix overflow in sliding_average in the case of very high download rates --- include/libtorrent/aux_/suggest_piece.hpp | 2 +- include/libtorrent/block_cache.hpp | 1 - include/libtorrent/disk_io_thread.hpp | 1 - include/libtorrent/peer_connection.hpp | 2 +- include/libtorrent/receive_buffer.hpp | 7 ++++--- include/libtorrent/sliding_average.hpp | 23 +++++++++++++---------- src/receive_buffer.cpp | 6 +++--- src/utp_stream.cpp | 2 +- test/test_receive_buffer.cpp | 11 +++++++++++ test/test_sliding_average.cpp | 12 ++++++------ 10 files changed, 40 insertions(+), 27 deletions(-) diff --git a/include/libtorrent/aux_/suggest_piece.hpp b/include/libtorrent/aux_/suggest_piece.hpp index a2af61bb5..cfb4c9a00 100644 --- a/include/libtorrent/aux_/suggest_piece.hpp +++ b/include/libtorrent/aux_/suggest_piece.hpp @@ -119,7 +119,7 @@ private: // have higher priority vector m_priority_pieces; - sliding_average<30> m_availability; + sliding_average m_availability; }; }} diff --git a/include/libtorrent/block_cache.hpp b/include/libtorrent/block_cache.hpp index 45fee2e59..dd0236e92 100644 --- a/include/libtorrent/block_cache.hpp +++ b/include/libtorrent/block_cache.hpp @@ -47,7 +47,6 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/error_code.hpp" #include "libtorrent/io_service_fwd.hpp" #include "libtorrent/hasher.hpp" -#include "libtorrent/sliding_average.hpp" #include "libtorrent/tailqueue.hpp" #include "libtorrent/linked_list.hpp" #include "libtorrent/disk_buffer_pool.hpp" diff --git a/include/libtorrent/disk_io_thread.hpp b/include/libtorrent/disk_io_thread.hpp index 5212f393f..62bbcdda2 100644 --- a/include/libtorrent/disk_io_thread.hpp +++ b/include/libtorrent/disk_io_thread.hpp @@ -39,7 +39,6 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/storage.hpp" #include "libtorrent/allocator.hpp" #include "libtorrent/io_service.hpp" -#include "libtorrent/sliding_average.hpp" #include "libtorrent/disk_io_thread_pool.hpp" #include "libtorrent/disk_io_job.hpp" #include "libtorrent/disk_job_pool.hpp" diff --git a/include/libtorrent/peer_connection.hpp b/include/libtorrent/peer_connection.hpp index 42fda5713..41cf63e86 100644 --- a/include/libtorrent/peer_connection.hpp +++ b/include/libtorrent/peer_connection.hpp @@ -842,7 +842,7 @@ namespace aux { // outstanding request, the time since the piece was requested. It // is essentially an estimate of the time it will take to completely // receive a payload message after it has been requested. - sliding_average<20> m_request_time; + sliding_average m_request_time; // keep the io_service running as long as we // have peer connections diff --git a/include/libtorrent/receive_buffer.hpp b/include/libtorrent/receive_buffer.hpp index b03355e78..4c78c24ac 100644 --- a/include/libtorrent/receive_buffer.hpp +++ b/include/libtorrent/receive_buffer.hpp @@ -36,6 +36,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/buffer.hpp" #include "libtorrent/disk_buffer_holder.hpp" #include "libtorrent/sliding_average.hpp" +#include "libtorrent/aux_/numeric_cast.hpp" #include @@ -57,8 +58,8 @@ struct TORRENT_EXTRA_EXPORT receive_buffer bool packet_finished() const { return m_packet_size <= m_recv_pos; } int pos() const { return m_recv_pos; } - int capacity() const { return int(m_recv_buffer.size()); } - int watermark() const { return m_watermark.mean(); } + int capacity() const { return aux::numeric_cast(m_recv_buffer.size()); } + int watermark() const { return aux::numeric_cast(m_watermark.mean()); } span reserve(int size); void grow(int limit); @@ -154,7 +155,7 @@ private: // keep track of how much of the receive buffer we use, if we're not using // enough of it we shrink it - sliding_average<20> m_watermark; + sliding_average m_watermark; buffer m_recv_buffer; }; diff --git a/include/libtorrent/sliding_average.hpp b/include/libtorrent/sliding_average.hpp index 6fd6f9d4d..d7560d4ac 100644 --- a/include/libtorrent/sliding_average.hpp +++ b/include/libtorrent/sliding_average.hpp @@ -35,26 +35,29 @@ POSSIBILITY OF SUCH DAMAGE. #include #include // for std::abs +#include + +#include "libtorrent/assert.hpp" namespace libtorrent { // an exponential moving average accumulator. Add samples to it and it keeps // track of a moving mean value and an average deviation -template +template struct sliding_average { + static_assert(std::is_integral::value, "template argument must be integral"); + sliding_average(): m_mean(0), m_average_deviation(0), m_num_samples(0) {} sliding_average(sliding_average const&) = default; sliding_average& operator=(sliding_average const&) = default; - void add_sample(int s) + void add_sample(Int s) { + TORRENT_ASSERT(s < std::numeric_limits::max() / 64); // fixed point s *= 64; - int deviation = 0; - - if (m_num_samples > 0) - deviation = std::abs(m_mean - s); + Int const deviation = (m_num_samples > 0) ? std::abs(m_mean - s) : 0; if (m_num_samples < inverted_gain) ++m_num_samples; @@ -70,14 +73,14 @@ struct sliding_average } } - int mean() const { return m_num_samples > 0 ? (m_mean + 32) / 64 : 0; } - int avg_deviation() const { return m_num_samples > 1 ? (m_average_deviation + 32) / 64 : 0; } + Int mean() const { return m_num_samples > 0 ? (m_mean + 32) / 64 : 0; } + Int avg_deviation() const { return m_num_samples > 1 ? (m_average_deviation + 32) / 64 : 0; } int num_samples() const { return m_num_samples; } private: // both of these are fixed point values (* 64) - int m_mean = 0; - int m_average_deviation = 0; + Int m_mean = 0; + Int m_average_deviation = 0; // the number of samples we have received, but no more than inverted_gain // this is the effective inverted_gain int m_num_samples = 0; diff --git a/src/receive_buffer.cpp b/src/receive_buffer.cpp index 5e39fa93a..2e83f49c7 100644 --- a/src/receive_buffer.cpp +++ b/src/receive_buffer.cpp @@ -60,7 +60,7 @@ span receive_buffer::reserve(int const size) // since we just increased the size of the buffer, reset the watermark to // start at our new size (avoid flapping the buffer size) - m_watermark = sliding_average<20>(); + m_watermark = {}; } return aux::typed_span(m_recv_buffer).subspan(m_recv_end, size); @@ -83,7 +83,7 @@ void receive_buffer::grow(int const limit) // since we just increased the size of the buffer, reset the watermark to // start at our new size (avoid flapping the buffer size) - m_watermark = sliding_average<20>(); + m_watermark = {}; } int receive_buffer::advance_pos(int const bytes) @@ -180,7 +180,7 @@ void receive_buffer::normalize(int const force_shrink) // if the running average drops below half of the current buffer size, // reallocate a smaller one. - bool const shrink_buffer = int(m_recv_buffer.size()) / 2 > m_watermark.mean() + bool const shrink_buffer = std::int64_t(m_recv_buffer.size()) / 2 > m_watermark.mean() && m_watermark.mean() > (m_recv_end - m_recv_start); span bytes_to_shift(m_recv_buffer.data() + m_recv_start diff --git a/src/utp_stream.cpp b/src/utp_stream.cpp index accae1199..2d96cf459 100644 --- a/src/utp_stream.cpp +++ b/src/utp_stream.cpp @@ -472,7 +472,7 @@ struct utp_socket_impl std::int32_t m_recv_delay = 0; // average RTT - sliding_average<16> m_rtt; + sliding_average m_rtt; // if this is != 0, it means the upper layer provided a reason for why // the connection is being closed. The reason is indicated by this diff --git a/test/test_receive_buffer.cpp b/test/test_receive_buffer.cpp index ba5f9d56e..fd41f7440 100644 --- a/test/test_receive_buffer.cpp +++ b/test/test_receive_buffer.cpp @@ -211,6 +211,17 @@ TORRENT_TEST(receive_buffer_max_receive) TEST_EQUAL(b.max_receive(), max_receive - 20); } +TORRENT_TEST(receive_buffer_watermark) +{ + receive_buffer b; + b.reset(0x4000); + b.reserve(35000000); + b.received(35000000); + b.normalize(); + + TEST_EQUAL(b.watermark(), 35000000); +} + #if !defined(TORRENT_DISABLE_ENCRYPTION) && !defined(TORRENT_DISABLE_EXTENSIONS) TORRENT_TEST(recv_buffer_mutable_buffers) diff --git a/test/test_sliding_average.cpp b/test/test_sliding_average.cpp index 76b86917d..cbbd206c2 100644 --- a/test/test_sliding_average.cpp +++ b/test/test_sliding_average.cpp @@ -53,7 +53,7 @@ using namespace lt; // make sure we react quickly for the first few samples TORRENT_TEST(reaction_time) { - sliding_average<10> avg; + sliding_average avg; avg.add_sample(-10); avg.add_sample(10); @@ -63,7 +63,7 @@ TORRENT_TEST(reaction_time) TORRENT_TEST(reaction_time2) { - sliding_average<10> avg; + sliding_average avg; avg.add_sample(10); avg.add_sample(20); @@ -74,7 +74,7 @@ TORRENT_TEST(reaction_time2) // make sure we converge TORRENT_TEST(converge) { - sliding_average<10> avg; + sliding_average avg; avg.add_sample(100); for (int i = 0; i < 20; ++i) avg.add_sample(10); @@ -83,7 +83,7 @@ TORRENT_TEST(converge) TORRENT_TEST(converge2) { - sliding_average<10> avg; + sliding_average avg; avg.add_sample(-100); for (int i = 0; i < 20; ++i) avg.add_sample(-10); @@ -93,7 +93,7 @@ TORRENT_TEST(converge2) // test with a more realistic input TORRENT_TEST(random_converge) { - sliding_average<10> avg; + sliding_average avg; for (int i = 0; i < int(sizeof(samples)/sizeof(samples[0])); ++i) avg.add_sample(samples[i]); TEST_CHECK(abs(avg.mean() - 60) <= 3); @@ -101,7 +101,7 @@ TORRENT_TEST(random_converge) TORRENT_TEST(sliding_average) { - sliding_average<4> avg; + sliding_average avg; TEST_EQUAL(avg.mean(), 0); TEST_EQUAL(avg.avg_deviation(), 0); avg.add_sample(500);