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

TCP_NODELAY setsockopt uses BYTE on Windows when it should use BOOL #94092

Closed
chrisnc opened this issue Feb 17, 2022 · 2 comments · Fixed by #94094
Closed

TCP_NODELAY setsockopt uses BYTE on Windows when it should use BOOL #94092

chrisnc opened this issue Feb 17, 2022 · 2 comments · Fixed by #94094
Assignees
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@chrisnc
Copy link
Contributor

chrisnc commented Feb 17, 2022

This bug was found and mitigated by the Wine project here:
https://source.winehq.org/git/wine.git/commit/d6ea38f32dfd3edbe107a255c37e9f7f3da06ae7

The types for each setsockopt are documented here:
https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-setsockopt

As the Wine commit explains, Windows itself is more liberal in what values of optlen it will accept for boolean arguments, which is why this has not affected Windows proper, but it would still be better to pass the expected type and length, so that the Rust bindings will also work on more strict implementations of winsock, such as Wine's prior to this fix. I confirmed that this is the case using Wine 7.0, which is the current stable branch available from https://wiki.winehq.org/Debian. Wine 5.0.3, which is the version that Debian/Ubuntu distribute themselves, is not affected by this problem.

I tried this code:

use std::io::{self, Write};
use std::net::TcpStream;

fn main() -> io::Result<()> {
    let mut stream = TcpStream::connect("127.0.0.1:8080")?;
    stream.set_nodelay(true)?;
    stream.write(b"hello")?;
    Ok(())
}

I set up a listening peer with nc -l -p 8080,
built with cargo build --target x86_64-pc-windows-gnu,
and ran with wine target/x86_64-pc-windows-gnu/debug/tcp-nodelay-text.exe.

I expected to see this happen: all the socket operations succeed, and nc prints "hello" and exits.

Instead, this happened: set_nodelay returns an Err:

Error: Os { code: 10022, kind: InvalidInput, message: "OS Error 10022 (FormatMessageW() returned error 317)" }

If I run with strace -f wine target/x86_64-pc-windows-gnu/debug/tcp-nodelay-text.exe, which shows the Linux system calls that wine makes on behalf of the program, I can see:

<pid> setsockopt(9, SOL_TCP, TCP_NODELAY, "\1", 1)         = -1 EINVAL (Invalid argument)

I tested this using rust 1.58.1, but I confirmed that the code in question is still doing the same thing on the main branch of rust.

Here is a commit which should fix the issue:
chrisnc@33c1de8

I was able to test it myself and confirmed that the error goes away:

$ rustc +stage2 --target x86_64-pc-windows-gnu src/main.rs # the rustc+std that I built with this change
$ wine main.exe # works
$ rustc --target x86_64-pc-windows-gnu src/main.rs # rustc 1.58.1
$ wine main.exe # fails
Error: Os { code: 10022, kind: InvalidInput, message: "OS Error 10022 (FormatMessageW() returned error 317)" }

That this error isn't formatted correctly is probably another bug, but I haven't looked into it.

@chrisnc chrisnc added the C-bug Category: This is a bug. label Feb 17, 2022
@ChrisDenton
Copy link
Contributor

Thanks for reporting this! Would you mind creating a pull request with your changes? The rustc dev guide has information on the rustc PR workflow.

@chrisnc
Copy link
Contributor Author

chrisnc commented Feb 17, 2022

Sure thing: #94094

@dtolnay dtolnay added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 17, 2022
@dtolnay dtolnay linked a pull request Feb 17, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants