Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix infinite loop when announcing #4286

Merged
merged 1 commit into from
Jan 26, 2020

Conversation

ssiloti
Copy link
Collaborator

@ssiloti ssiloti commented Jan 25, 2020

In torrent::announce_with_tracker there is a check after announcing all of
a tracker's endpoints to see if all required announces have already been sent
according to the tier settings.

	if (std::all_of(listen_socket_states.begin(), listen_socket_states.end()
		, [](announce_state const& s) { return s.done; }))
		break;

Commit ab07ece added a check which can cause listen_socket_states to not be
populated if all endpoints for the first tracker are disabled. This causes the
early break out code above to trigger and prevent any announces from being sent.

The extra check is not in torrent::update_tracker_timer though, so if it thinks there
is at least one endpoint ready to announce it will immediately trigger another
on_tracker_announce, again, and again.

Simple fix is to move the offending check below the code which populates
listen_socket_states so the the latter will execute unconditionally.

In torrent::announce_with_tracker there is a check after announcing all of
a tracker's endpoints to see if all required announces have already been sent
according to the tier settings.

	if (std::all_of(listen_socket_states.begin(), listen_socket_states.end()
		, [](announce_state const& s) { return s.done; }))
		break;

Commit ab07ece added a check which can cause listen_socket_states to not be
populated if all endpoints for the first tracker are disabled. This causes the
early break out code above to trigger and prevent any announces from being sent.

The extra check is not in torrent::update_tracker_timer though, so if it thinks there
is at least one endpoint ready to announce it will immediately trigger another
on_tracker_announce, again, and again.

Simple fix is to move the offending check below the code which populates
listen_socket_states so the the latter will execute unconditionally.
@codecov-io
Copy link

Codecov Report

Merging #4286 into RC_1_2 will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           RC_1_2    #4286      +/-   ##
==========================================
+ Coverage   74.18%   74.35%   +0.17%     
==========================================
  Files         438      438              
  Lines       60719    60717       -2     
  Branches     8975     8975              
==========================================
+ Hits        45044    45148     +104     
+ Misses      11181    11063     -118     
- Partials     4494     4506      +12
Impacted Files Coverage Δ
src/torrent.cpp 60% <100%> (-0.13%) ⬇️
src/timestamp_history.cpp 92.59% <0%> (-3.71%) ⬇️
src/kademlia/routing_table.cpp 71.57% <0%> (-3.38%) ⬇️
src/close_reason.cpp 17.77% <0%> (-2.23%) ⬇️
src/http_tracker_connection.cpp 63.32% <0%> (-1.94%) ⬇️
src/alert.cpp 54.36% <0%> (-1.86%) ⬇️
test/web_seed_suite.cpp 89% <0%> (-1.5%) ⬇️
include/libtorrent/alert_types.hpp 54.94% <0%> (-1.1%) ⬇️
src/udp_tracker_connection.cpp 54.74% <0%> (-0.84%) ⬇️
src/choker.cpp 21.95% <0%> (-0.82%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4054cbb...a1790db. Read the comment docs.

@arvidn
Copy link
Owner

arvidn commented Jan 26, 2020

thanks! Sometimes I think about rewriting the whole announce logic to more directly express the logic, and be bit more robust (this is not the first time there's been an infinite loop bug in this code). But there's quite a lot of behavior that would need to preserved, so it's not that easy.

@arvidn arvidn merged commit cc8cbca into arvidn:RC_1_2 Jan 26, 2020
@ssiloti ssiloti deleted the announce-inf-loop-fix branch May 29, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants