From 16249b8135a23931e1a7104392e65231ebaaaaa2 Mon Sep 17 00:00:00 2001 From: arvidn Date: Thu, 6 Sep 2018 19:09:09 -0700 Subject: [PATCH] fix overflow in calc_bytes(), add more tests, fix bug in piece picker accounting of filtered pad blocks. --- src/piece_picker.cpp | 30 ++++++++++++-- src/torrent.cpp | 4 +- test/test_piece_picker.cpp | 85 ++++++++++++++++++++++++++++++++++++++ test/test_torrent.cpp | 4 +- 4 files changed, 115 insertions(+), 8 deletions(-) diff --git a/src/piece_picker.cpp b/src/piece_picker.cpp index 8647516ea..dda267952 100644 --- a/src/piece_picker.cpp +++ b/src/piece_picker.cpp @@ -3526,6 +3526,12 @@ get_out: ++m_pads_in_piece[block.piece_index]; + piece_pos& p = m_piece_map[block.piece_index]; + if (p.filtered()) + { + ++m_filtered_pad_blocks; + } + // if we mark and entire piece as a pad file, we need to also // consder that piece as "had" and increment some counters int const blocks = blocks_in_piece(block.piece_index); @@ -3664,33 +3670,49 @@ get_out: piece_count piece_picker::want() const { bool const want_last = piece_priority(piece_index_t(num_pieces() - 1)) != dont_download; - return { num_pieces() - m_num_filtered - m_num_have_filtered + piece_count ret{ num_pieces() - m_num_filtered - m_num_have_filtered , num_pad_blocks() - m_filtered_pad_blocks - m_have_filtered_pad_blocks , want_last }; + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.last_piece == true)); + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.pad_blocks > 0)); + TORRENT_ASSERT(!(ret.num_pieces == num_pieces() && ret.last_piece == false)); + return ret; } piece_count piece_picker::have_want() const { bool const have_last = have_piece(piece_index_t(num_pieces() - 1)); bool const want_last = piece_priority(piece_index_t(num_pieces() - 1)) != dont_download; - return { m_num_have - m_num_have_filtered + piece_count ret{ m_num_have - m_num_have_filtered , m_have_pad_blocks - m_have_filtered_pad_blocks , have_last && want_last }; + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.last_piece == true)); + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.pad_blocks > 0)); + TORRENT_ASSERT(!(ret.num_pieces == num_pieces() && ret.last_piece == false)); + return ret; } piece_count piece_picker::have() const { bool const have_last = have_piece(piece_index_t(num_pieces() - 1)); - return { m_num_have + piece_count ret{ m_num_have , m_have_pad_blocks , have_last }; + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.last_piece == true)); + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.pad_blocks > 0)); + TORRENT_ASSERT(!(ret.num_pieces == num_pieces() && ret.last_piece == false)); + return ret; } piece_count piece_picker::all_pieces() const { - return { num_pieces() + piece_count ret{ num_pieces() , num_pad_blocks() , true}; + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.last_piece == true)); + TORRENT_ASSERT(!(ret.num_pieces == 0 && ret.pad_blocks > 0)); + TORRENT_ASSERT(!(ret.num_pieces == num_pieces() && ret.last_piece == false)); + return ret; } } diff --git a/src/torrent.cpp b/src/torrent.cpp index 7a755429d..48334a114 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -3478,10 +3478,10 @@ bool is_downloading_state(int const st) int const block_size = std::min(default_block_size, fs.piece_length()); // every block should not be a pad block - TORRENT_ASSERT(pc.pad_blocks <= pc.num_pieces * fs.piece_length() / block_size); + TORRENT_ASSERT(pc.pad_blocks <= std::int64_t(pc.num_pieces) * fs.piece_length() / block_size); return std::int64_t(pc.num_pieces) * fs.piece_length() - - (pc.last_piece ? 1 : 0) * (fs.piece_length() - fs.piece_size(fs.last_piece())) + - (pc.last_piece ? fs.piece_length() - fs.piece_size(fs.last_piece()) : 0) - std::int64_t(pc.pad_blocks) * block_size; } diff --git a/test/test_piece_picker.cpp b/test/test_piece_picker.cpp index d36a7a7b5..799880c4b 100644 --- a/test/test_piece_picker.cpp +++ b/test/test_piece_picker.cpp @@ -2061,5 +2061,90 @@ TORRENT_TEST(pad_blocks_in_last_piece) TEST_EQUAL(p->pad_blocks_in_piece(piece_index_t{0}), 0); } +namespace { +void validate_piece_count(piece_count const& c) +{ + // it's an impossible combination to have 0 pieces, but still have one of them be the last piece + TEST_CHECK(!(c.num_pieces == 0 && c.last_piece == true)); + + // if we have 0 pieces, we can't have any pad blocks either + TEST_CHECK(!(c.num_pieces == 0 && c.pad_blocks > 0)); + + // if we have all pieces, we must also have the last one + TEST_CHECK(!(c.num_pieces == 4 && c.last_piece == false)); +} + +void validate_all_pieces(piece_count const& c) +{ + TEST_EQUAL(c.last_piece, true); + TEST_EQUAL(c.num_pieces, 4); + TEST_EQUAL(c.pad_blocks, 3); +} + +void validate_no_pieces(piece_count const& c) +{ + TEST_EQUAL(c.last_piece, false); + TEST_EQUAL(c.num_pieces, 0); + TEST_EQUAL(c.pad_blocks, 0); +} +} + +TORRENT_TEST(pad_blocks_all_filtered) +{ + auto p = setup_picker("1111", " ", "0000", ""); + p->mark_as_pad({piece_index_t{1}, 0}); + p->mark_as_pad({piece_index_t{1}, 1}); + p->mark_as_pad({piece_index_t{2}, 0}); + + validate_piece_count(p->all_pieces()); + validate_piece_count(p->have()); + validate_piece_count(p->have_want()); + validate_piece_count(p->want()); + + validate_all_pieces(p->all_pieces()); + validate_no_pieces(p->have()); + validate_no_pieces(p->have_want()); + validate_no_pieces(p->want()); +} + +TORRENT_TEST(pad_blocks_all_wanted) +{ + auto p = setup_picker("1111", " ", "4444", ""); + p->mark_as_pad({piece_index_t{1}, 0}); + p->mark_as_pad({piece_index_t{1}, 1}); + p->mark_as_pad({piece_index_t{2}, 0}); + + validate_piece_count(p->all_pieces()); + validate_piece_count(p->have()); + validate_piece_count(p->have_want()); + validate_piece_count(p->want()); + + validate_all_pieces(p->all_pieces()); + validate_all_pieces(p->want()); + validate_no_pieces(p->have()); + validate_no_pieces(p->have_want()); +} + +TORRENT_TEST(pad_blocks_some_wanted) +{ + auto p = setup_picker("1111", " ", "0404", ""); + p->mark_as_pad({piece_index_t{1}, 0}); + p->mark_as_pad({piece_index_t{1}, 1}); + p->mark_as_pad({piece_index_t{2}, 0}); + + validate_piece_count(p->all_pieces()); + validate_piece_count(p->have()); + validate_piece_count(p->have_want()); + validate_piece_count(p->want()); + + validate_all_pieces(p->all_pieces()); + validate_no_pieces(p->have()); + validate_no_pieces(p->have_want()); + + TEST_EQUAL(p->want().num_pieces, 2); + TEST_EQUAL(p->want().last_piece, true); + TEST_EQUAL(p->want().pad_blocks, 2); +} + //TODO: 2 test picking with partial pieces and other peers present so that both // backup_pieces and backup_pieces2 are used diff --git a/test/test_torrent.cpp b/test/test_torrent.cpp index fe2d263c0..46ce67a04 100644 --- a/test/test_torrent.cpp +++ b/test/test_torrent.cpp @@ -694,13 +694,13 @@ TORRENT_TEST(test_read_piece_out_of_range) } namespace { -int const piece_size = 0x4000 * 2; +int const piece_size = 0x4000 * 128; file_storage test_fs() { file_storage fs; fs.set_piece_length(piece_size); - fs.add_file("temp", 999999); + fs.add_file("temp", 99999999999); fs.set_num_pieces(int((fs.total_size() + piece_size - 1) / piece_size)); return fs; }