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.

Sign up for GitHub

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

Jump to bottom

Update comment regarding SO_REUSEADDR on Windows #71785

Merged
merged 1 commit into from
May 3, 2020
Merged

Update comment regarding SO_REUSEADDR on Windows #71785

merged 1 commit into from
May 3, 2020

Conversation

reitermarkus
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2020
@reitermarkus
Copy link
Contributor Author

reitermarkus commented May 2, 2020 •
edited
Loading

Seems like SO_REUSEADDR actually exists on Windows: https://docs.microsoft.com/en-us/windows/win32/winsock/using-so-reuseaddr-and-so-exclusiveaddruse

@reitermarkus reitermarkus changed the title Replace cfg macro with attribute. Remove cfg attribute. May 2, 2020
@jonas-schievink jonas-schievink added T-libs Relevant to the library team, which will review and decide on the PR/issue. O-windows Operating system: Windows labels May 2, 2020
@Mark-Simulacrum
Copy link
Member

Hm, so some rough searching online suggests that Windows may not actually want this set (e.g. this random github thread).

I myself don't have much windows experience so I'm not willing to approve this just yet. I'll leave myself assigned as I don't know if we have anyone better at this time.

cc @retep998

@reitermarkus
Copy link
Contributor Author

I also don't really have any Windows experience, just stumbled upon this while implementing libstd for another target.

So either this should be used or the constant removed.

@retep998
Copy link
Member

retep998 commented May 2, 2020 •
edited
Loading

From the very MSDN article that you linked:

The SO_REUSEADDR option has very few uses in normal applications aside from multicast sockets where data is delivered to all of the sockets bound on the same port. Otherwise, any application that sets this socket option should be redesigned to remove the dependency since it is eminently vulnerable to "socket hijacking". As long as SO_REUSEADDR socket option can be used to potentially hijack a port in a server application, the application must be considered to be not secure.

While on non-windows platforms SO_REUSEADDR allows you to bind to sockets that are still in TIME_WAIT, on Windows SO_REUSEADDR allows you to bind to sockets that are actively being used resulting in undefined behavior.

So no, we do not want this change. A change which we should consider though is setting SO_EXCLUSIVEADDRUSE for all our sockets on Windows.

@Mark-Simulacrum
Copy link
Member

Okay, yeah, that's what I suspected based on the GitHub thread. My skimming must have missed that on the MSDN page.

I would appreciate an update to this PR which includes a note that links to the MSDN page and summarizes why we don't want it.

@tesuji
Copy link
Contributor

tesuji commented May 2, 2020 •
edited
Loading

This PR could be changed to document that for Windows.

@reitermarkus reitermarkus changed the title Remove cfg attribute. Replace cfg macro with attribute. May 2, 2020
@reitermarkus
Copy link
Contributor Author

Ok, I reverted the change and added an explanation as to why we don't set it on Windows.

@Mark-Simulacrum Mark-Simulacrum changed the title Replace cfg macro with attribute. Update comment regarding SO_REUSEADDR on Windows May 2, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 2, 2020

📌 Commit 39a9790 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2020
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 2, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2020
@bors
Auto merge of rust-lang#71807 - Dylan-DPC:rollup-jvb8sd9, r=Dylan-DPC
c1e0552 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#69274 (Implement RFC 2396: `#[target_feature]` 1.1)
 - rust-lang#71767 (doc: make Stack and StackElement a little pretty)
 - rust-lang#71772 (Mark query function as must_use.)
 - rust-lang#71777 (cleanup: `config::CrateType` -> `CrateType`)
 - rust-lang#71784 (Remove recommendation for unmaintained dirs crate)
 - rust-lang#71785 (Update comment regarding SO_REUSEADDR on Windows)
 - rust-lang#71787 (fix rustdoc warnings)

Failed merges:

r? @ghost
@bors bors merged commit 5a7b21f into rust-lang:master May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@reitermarkus @rust-highfive @Mark-Simulacrum @retep998 @tesuji @bors @jonas-schievink