Skip to content

Conversation

youknowone
Copy link
Member

No description provided.

return None;
match bytes.and_then(|v| v.to_usize()) {
Some(bytes) => {
let mut buffer = Vec::with_capacity(bytes);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is safe? For fileio I think we just do a vec![0; bufsize]

Copy link
Member Author

@youknowone youknowone Aug 2, 2020

Choose a reason for hiding this comment

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

The length is set by next line unsafe set_len and with_capacity just help it a bit. If read_exact has any problem, regardless of zero-filled vector or garbage filled vector(with set_len), it causes the same problem. (But I believe read_exact will work corrent)

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, though I'd prefer a vector with zeroes rather than UB, but if you think this is fine than feel free to use it.

Copy link
Member Author

@youknowone youknowone Aug 2, 2020

Choose a reason for hiding this comment

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

This is neither UB nor unsafe. Please tell me if I am missing something.

Vec<T>::set_len is a vector version of MaybeUninit, so it may cause UB by the element type T and capaticy. In this case, the type T is u8. so it does not cause any type related unsafe case. About the capacity, we set it in this line. ("new_len must be less than or equal to capacity()." from https://doc.rust-lang.org/std/vec/struct.Vec.html#method.set_len) So this is successfully avoiding both of the unsafe case.
By meeting both of the criteria of set_len, even in the worst case, the result is just garbage bytes in vector. Any wrong u8 value in safe vector is not related to UB.

The next steps are also guaranteed to be safe.
In document, the Read trait requires (though it is not forced, I believe std followed the requirements) the buffer is expected to be read-only. ("It is recommended that implementations only write data to buf instead of reading its contents." from https://doc.rust-lang.org/std/io/trait.Read.html#method.read_exact)
So, we don't need to set it to any value because read_exact never use it. And after calling read_exact successfully, the vector will be full of new read data; otherwise the result will be Err.

Because set_len itself is not safe, I will put with_capacity into unsafe block and add a comment they must be coupled forever to help other people who will read this code next time.

@coolreader18 coolreader18 merged commit 7a77dbc into RustPython:master Aug 2, 2020
@youknowone youknowone deleted the buffered-io branch August 2, 2020 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants