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

call out explicitly that general read needs to be called with an initialized buffer #62102

Merged
merged 2 commits into from Jun 28, 2019

Conversation

@RalfJung
Copy link
Member

commented Jun 24, 2019

No description provided.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

r? @KodrAus

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

@Shnatsel

This comment has been minimized.

Copy link

commented Jun 24, 2019

As a casual Rust developer, it is not clear to me who a "user" of a trait is in this context.

How about "responsibility of the code calling this function?"

/// so it is perfectly possible that the implementation might inspect that data.
/// As a caller, it is your responsibility to make sure that `buf` is initialized
/// before calling `read`. Calling `read` with an uninitialized `buf` (of the kind one
/// obtains via [`MaybeUninit<T>`]) is not safe, and can lead to undefined behavior.

This comment has been minimized.

Copy link
@Shnatsel

Shnatsel Jun 24, 2019

I would replace this entire paragraph with the following:

However, the trait is safe to implement, so it is possible that the code that's supposed to write to the buffer might also read from it. It is your responsibility to make sure that `buf` is initialized before calling `read`. Calling `read` with an uninitialized `buf` (of the kind one obtains via [`MaybeUninit<T>`]) is not safe, and can lead to undefined behavior.

This comment has been minimized.

Copy link
@sfackler

sfackler Jun 24, 2019

Member

This all depends on how the Read implementation's initializer method behaves.

This comment has been minimized.

Copy link
@KodrAus

KodrAus Jun 25, 2019

Contributor

As general advice this sounds good to me, since the initializer API is unstable I don’t know how useful it would be for an end-user for us to mention it here just yet.

As far as I understand passing an uninitialized buffer to a ‘well-behaved’ Read (which is a property inherited from any Reads it may wrap) shouldn’t invoke UB, which isn’t enforced through the type, but may be achieved in codebases that write all their own Read implementations.

This comment has been minimized.

Copy link
@Centril

Centril Jun 25, 2019

Member

@KodrAus This depends on the outcome of rust-lang/unsafe-code-guidelines#77. For now, you should assume that &mut place asserts that:

does &T have to point to allocated memory (with at least size_of::() bytes being allocated)? If yes, does the memory have to contain data that satisfies the validity invariant of T?

are both true.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jun 25, 2019

Author Member

Good point about user vs. caller, and I also took some of your other wording changes.

This all depends on how the Read implementation's initializer method behaves.

I'm aware, but should the docs of a stable method really refer to an unstable one here?

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Jul 4, 2019

Contributor

Maybe this would be clearer with an example of a problematic implementation. "It is allowed for someone to have an impl that looks like ... If that impl mean that your code is UB for example by passing an uninitialized buf (of the kind one obtains via [MaybeUninit<T>]) then your code is incorrect."

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 7, 2019

Author Member

@Eh2406 sure, would you like to try to write something like that and submit it as a PR?

This comment has been minimized.

Copy link
@Eh2406

Eh2406 Jul 7, 2019

Contributor

I would like to, I can add it to my list, but I doubt I will get to it. :-( I am already backlogged with Cargo and Dayjob work. So much to do, so little time.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 7, 2019

Author Member

I know that feeling all too well. ;)

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

r=me rollup if you like, possibly with s/users/consumers re. @Shnatsel's concern.

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

@bors r=Centril rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

📌 Commit 390f717 has been approved by Centril

Centril added a commit to Centril/rust that referenced this pull request Jun 25, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 25, 2019

Auto merge of #62129 - Centril:rollup-41vx5n4, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #61767 (Update new_debug_unreachable)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 26, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 26, 2019

Auto merge of #62139 - Centril:rollup-jvwtako, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #60340 (Rename .cap() methods to .capacity())
 - #61767 (Update new_debug_unreachable)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 26, 2019

Auto merge of #62139 - Centril:rollup-jvwtako, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #60340 (Rename .cap() methods to .capacity())
 - #61767 (Update new_debug_unreachable)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 26, 2019

Auto merge of #62139 - Centril:rollup-jvwtako, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #60340 (Rename .cap() methods to .capacity())
 - #61767 (Update new_debug_unreachable)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 26, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 27, 2019

Auto merge of #62148 - Centril:rollup-a8nm4gm, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #61767 (Update new_debug_unreachable)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62103 (Add debug assertions to write_bytes and copy*)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 27, 2019

Auto merge of #62178 - Centril:rollup-m9g77jr, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61767 (Update new_debug_unreachable)
 - #62043 (Remove `FnBox`)
 - #62067 (Add suggestion for missing `.await` keyword)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62103 (Add debug assertions to write_bytes and copy*)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 27, 2019

Auto merge of #62180 - Centril:rollup-g5jeoew, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62103 (Add debug assertions to write_bytes and copy*)
 - #62106 (Add more tests for async/await)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 27, 2019

Auto merge of #62180 - Centril:rollup-g5jeoew, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62103 (Add debug assertions to write_bytes and copy*)
 - #62106 (Add more tests for async/await)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 27, 2019

Auto merge of #62182 - Centril:rollup-reguimn, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62067 (Add suggestion for missing `.await` keyword)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019

Rollup merge of rust-lang#62102 - RalfJung:read, r=Centril
call out explicitly that general read needs to be called with an initialized buffer

bors added a commit that referenced this pull request Jun 27, 2019

Auto merge of #62192 - Centril:rollup-kssnlta, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62067 (Add suggestion for missing `.await` keyword)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 28, 2019

Auto merge of #62192 - Centril:rollup-kssnlta, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62067 (Add suggestion for missing `.await` keyword)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jun 28, 2019

Auto merge of #62192 - Centril:rollup-kssnlta, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #61878 (improve pinning projection docs)
 - #62043 (Remove `FnBox`)
 - #62067 (Add suggestion for missing `.await` keyword)
 - #62076 (Updated RELEASES.md for 1.36.0)
 - #62102 (call out explicitly that general read needs to be called with an initialized buffer)
 - #62106 (Add more tests for async/await)
 - #62124 (refactor lexer to use idiomatic borrowing)
 - #62131 (libsyntax: Fix some Clippy warnings)
 - #62152 (Don't ICE on item in `.await` expression)
 - #62154 (Remove old fixme)
 - #62155 (Add regression test for MIR drop generation in async loops)
 - #62156 (Update books)
 - #62160 (Remove outdated question_mark_macro_sep lint)
 - #62164 (save-analysis: use buffered writes)
 - #62171 (rustc: Retry SIGILL linker invocations)
 - #62176 (Update RLS)

Failed merges:

r? @ghost

@bors bors merged commit 390f717 into rust-lang:master Jun 28, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
pr Build #20190625.51 succeeded
Details

@RalfJung RalfJung deleted the RalfJung:read branch Jul 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.