use BOOL for TCP_NODELAY setsockopt value on Windows

[chrisnc]

This issue was found by the Wine project and mitigated there ¹.

Windows' setsockopt expects a BOOL (a typedef for int) for TCP_NODELAY
². Windows itself is forgiving and will accept any positive optlen and
interpret the first byte of *optval as the value, so this bug does not
affect Windows itself, but does affect systems implementing Windows'
interface more strictly, such as Wine. Wine was previously passing this
through to the host's setsockopt, where, e.g., Linux requires that
optlen be correct for the chosen option, and TCP_NODELAY expects an int.

Footnotes

1. https://source.winehq.org/git/wine.git/commit/d6ea38f32dfd3edbe107a255c37e9f7f3da06ae7 ↩

2. https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-setsockopt ↩

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

Please see the contribution instructions for more information.

[chrisnc]

Fixed footnote index in commit message...

[dtolnay]

Thanks!

[dtolnay]

@bors r+

[bors]

📌 Commit e5f7239 has been approved by dtolnay

[chrisnc]

I guess I didn't properly link this commit to #94092, since I made the commit before creating the issue. Should I update the commit message again to do so, or can someone with write perms link it for me? https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#manually-linking-a-pull-request-to-an-issue

Edit: never mind; I just updated the PR description. I think that should do it.

Edit the second: thanks!

[matthiaskrgr]

@bors r-
looks like this causes problems on ci: #94152 (comment)

[chrisnc]

Agh, looks like the assertion in the getsockopt wrapper is complaining about the size. The docs say that the optval for TCP_NODELAY on getsockopt should also be BOOL (int), but this test failure suggests that getsockopt is setting *optlen to 1... I will investigate.

[chrisnc]

So, I had tested the getsockopt change previously, but I was only testing on Wine, which does not modify *optlen and hence doesn't trigger this assert_eq... This makes me think that this assertion is too strict. POSIX allows for *optlen to be set to a lower value than what is passed in, and Windows is clearly doing this in reality (setting it to 1 after 4 is passed in). It worked before because Rust was always passing in 1, which strictly speaking is not correct but Windows accepts anyway. What do you think @dtolnay?

[chrisnc]

Somewhat related, I noticed that the getsockopt and setsockopt wrappers in sys_common/net.rs have incorrect argument names, compared with the corresponding names on the C interface. What the wrapper calls opt is actually level, val is actually option_name, and then because it already used val for the wrong thing, it calls the option value payload in setsockopt and slot in getsockopt. I'd like to change these.

[ChrisDenton]

So to sum up the issue:

• The data for TCP_NODELAY is documented as being a BOOL (a 32 bit signed integer)
• Microsoft Windows will only actually read or write the first byte.
• WINE used the full 4 bytes.
• The specification for getsockopt/setsockopt is flexible and allows for either implementation.

Is that about right?

[chrisnc]

@ChrisDenton yes that's most of it. I believe that this patch as-is does fix setsockopt on all versions of Wine while still working correctly for Windows, but now the problem is getsockopt, where Rust asserts that the output value of *option_len (the POSIX name for it) does not change.

Windows will accept >= 1 byte for setsockopt and getsockopt, and then for getsockopt, it will set *optlen (the Windows name for it) to 1, even if it was passed in as 4. (This is evident from the failing assertion in the test, but I'm still working on getting my Windows environment set up and getting a working example to reproduce this.) I've been in contact with the Wine maintainers who made the recent change, and they're also interested in matching the behavior of Windows on the final value of *optlen for getsockopt.

I have patches to fix up the parameter names and to remove the assertion, but I wanted to get some feedback on whether this is the right path. I can push them here for review shortly. I'm not sure if there's a better way to check for mismatched sizes in a way that is more permissive but still useful; the getsockopt interface is pretty cavalier about what happens when the sizes don't match. POSIX says that if *option_len is smaller than the size of the value, for example, that the value will just be truncated, and then if it's larger, that *option_len will be set to the actual (smaller) size, which is what Windows is doing here, even though Windows documents that the type is BOOL (int). Kind of a mess.

[chrisnc]

I've put two commits ahead of the original one. Appreciate any feedback, particularly on removing the assert.

[chrisnc]

r? rust-lang/libs

[dtolnay]

@bors r+

[bors]

📌 Commit b02698c has been approved by dtolnay