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

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

Merged
merged 2 commits into from
Jun 28, 2019
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c12b1b
call out explicitly that general read needs to be called with an init…
RalfJung Jun 24, 2019
390f717
tweak wording
RalfJung Jun 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/libstd/io/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,9 +506,18 @@ pub trait Read {
///
/// No guarantees are provided about the contents of `buf` when this
/// function is called, implementations cannot rely on any property of the
/// contents of `buf` being true. It is recommended that implementations
/// contents of `buf` being true. It is recommended that *implementations*
/// only write data to `buf` instead of reading its contents.
///
/// Correspondingly, however, *callers* of this method may not assume any guarantees
/// about how the implementation uses `buf`. 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.
Copy link
Member

@Shnatsel Shnatsel Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@KodrAus KodrAus Jun 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that feeling all too well. ;)

///
/// [`MaybeUninit<T>`]: ../mem/union.MaybeUninit.html
///
/// # Errors
///
/// If this function encounters any form of I/O or other error, an error
Expand Down