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: windows use of REUSEADDR #2790

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Nov 10, 2024

When SO_REUSEADDR is set on windows, it allows another process to "steal" the address, while in use.

see https://learn.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-and-so-exclusiveaddruse#using-so_exclusiveaddruse


This change is Reviewable

@Green-Sky Green-Sky added bug Bug fix for the user, not a fix to a build script network Network labels Nov 10, 2024
@Green-Sky Green-Sky added this to the v0.2.21 milestone Nov 10, 2024
@Green-Sky Green-Sky force-pushed the fix_window_reuseaddr branch 4 times, most recently from 1310df4 to eb3c8e4 Compare November 10, 2024 16:44
Copy link

codecov bot commented Nov 10, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.98%. Comparing base (fa723e3) to head (eb3c8e4).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
toxcore/network.c 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2790      +/-   ##
==========================================
+ Coverage   72.96%   72.98%   +0.01%     
==========================================
  Files         149      149              
  Lines       30566    30572       +6     
==========================================
+ Hits        22303    22313      +10     
+ Misses       8263     8259       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nurupo
Copy link
Member

nurupo commented Nov 10, 2024

Does it make sense to set both SO_REUSEADDR and SO_EXCLUSIVEADDRUSE socket options? From that documentation it sounds like one must use one or another.

@nurupo
Copy link
Member

nurupo commented Nov 10, 2024

Conversely, a socket with the SO_EXCLUSIVEADDRUSE set cannot necessarily be reused immediately after socket closure. For example, if a listening socket with SO_EXCLUSIVEADDRUSE set accepts a connection and is then subsequently closed, another socket (also with SO_EXCLUSIVEADDRUSE) cannot bind to the same port as the first socket until the original connection becomes inactive.

Tox clients failing to restart due to the toxcore being unable to bind to the same port until the original connection becomes inactive is the reason why SO_REUSEADDR was used in the first place. Setting SO_EXCLUSIVEADDRUSE brings this issue back again.

However, we made toxcore attempt to bind to a range of ports since then, 33445-33545 inclusively by default, so perhaps the use of a range of ports mitigates this issue and it's alright to use SO_EXCLUSIVEADDRUSE now?

@Green-Sky Green-Sky marked this pull request as ready for review November 11, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fix for the user, not a fix to a build script network Network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants