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

lack of documentation on the issue that using std::io::Write::write_all with a std::net::TcpStream results in undefined behavior when non_blocking(true) #115451

Closed
softstream-link opened this issue Sep 1, 2023 · 6 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-io Area: std::io, std::fs, std::net and std::path T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@softstream-link
Copy link

Location

lack of documentation on the issue that using std::io::Write::write_all with a std::net::TcpStream results in undefined behavior when non_blocking(true)

Summary

lack of documentation on the issue that using std::io::Write::write_all with a std::net::TcpStream results in undefined behavior when non_blocking(true)

@softstream-link softstream-link added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Sep 1, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 1, 2023
@ChrisDenton
Copy link
Member

It would help to have more details here. When you say "undefined behavior" are you meaning a Rust memory safety issue? Or inconsistent or unexpected behaviour?

Can you show some code that gives an example?

@softstream-link
Copy link
Author

softstream-link commented Sep 1, 2023

@ChrisDenton ChrisDenton added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-io Area: std::io, std::fs, std::net and std::path and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 1, 2023
@ChrisDenton
Copy link
Member

I don't think there's any UB in the standard library itself. There are a number of issues with the code, that helpful people on the user's forum have pointed out, but not with the standard library itself.

There might be a documentation issue here with using write_all and non-blocking TcpStream. It would help to have a very minimal example of that specific issue that can be used to inform users of the problem.

@scottmcm
Copy link
Member

scottmcm commented Sep 1, 2023

The steps to replicate contain the following as part of read_frame:

let mut buf: [u8; MAX_MESSAGE_SIZE] = unsafe { MaybeUninit::uninit().assume_init() };

That's 100% insta-UB, as the warning says in https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f15d743f0a7edd56eb2aca384838643e:

warning: the type `[u8; 10]` does not permit being left uninitialized
 --> src/main.rs:4:52
  |
4 |     let mut buf: [u8; MAX_MESSAGE_SIZE] = unsafe { MaybeUninit::uninit().assume_init() };
  |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                                                    |
  |                                                    this code causes undefined behavior when executed
  |                                                    help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
  |
  = note: integers must be initialized
  = note: `#[warn(invalid_value)]` on by default

So I'm going to close this as not a std issue.

Feel free to re-open with a minimal repro that doesn't contain UB.

(It's possible this appeared to work in past rust, but that when #111999 changed how we emit code for byte arrays it allowed LLVM to take advantage of the UB.)

@scottmcm scottmcm closed this as completed Sep 1, 2023
@softstream-link
Copy link
Author

softstream-link commented Sep 1, 2023

Sorry for some reason I don't see the re-open button for this issue. I am hoping you can reopen if you find below sufficient.

Here is the most basic block of code that will cause "UB" I don't want to split hair on what "UB" means but in my case it just means you don't get what you expect, hence you get something that Is not clearly defined.

let mut stream = TcpStream::connect(add).unwrap();
stream.nonblocking(true).unwrap()
let buf = [1,2,3,4,5];
stream.write_all(&buf[..]);  // THIS LINE WILL sometime propagate a WouldBlock error kind however it would have written X number of bytes to the steam with out a way too tell if X is =0 or X<buf.len()

@softstream-link
Copy link
Author

The steps to replicate contain the following as part of read_frame:

let mut buf: [u8; MAX_MESSAGE_SIZE] = unsafe { MaybeUninit::uninit().assume_init() };

That's 100% insta-UB, as the warning says in https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f15d743f0a7edd56eb2aca384838643e:

warning: the type `[u8; 10]` does not permit being left uninitialized
 --> src/main.rs:4:52
  |
4 |     let mut buf: [u8; MAX_MESSAGE_SIZE] = unsafe { MaybeUninit::uninit().assume_init() };
  |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                                                    |
  |                                                    this code causes undefined behavior when executed
  |                                                    help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
  |
  = note: integers must be initialized
  = note: `#[warn(invalid_value)]` on by default

So I'm going to close this as not a std issue.

Feel free to re-open with a minimal repro that doesn't contain UB.

(It's possible this appeared to work in past rust, but that when #111999 changed how we emit code for byte arrays it allowed LLVM to take advantage of the UB.)

changing this to SAFE init does not eliminate the issue of write_all not writing_all when the socket is notblocking, i am not sure exactly why it is unsafe to write to an uninitiated slice since the write from the socket will initialize it. I am just not aware of how else it can be done without the cost penalty, particularly since the read_buf(BorrowedCursor) 'suite' is still not stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-io Area: std::io, std::fs, std::net and std::path T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants