Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion tokio/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ features = [
tokio-test = { version = "0.4.0", path = "../tokio-test" }
tokio-stream = { version = "0.1", path = "../tokio-stream" }
futures = { version = "0.3.0", features = ["async-await"] }
mockall = "0.11.1"
mockall = "0.13.0"
async-stream = "0.3"
futures-concurrency = "7.6.3"

Expand Down
7 changes: 7 additions & 0 deletions tokio/src/fs/mocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ impl Read for MockFile {

impl Read for &'_ MockFile {
fn read(&mut self, dst: &mut [u8]) -> io::Result<usize> {
// Placate Miri. Tokio will call this method with an uninitialized
// buffer, which is ok because std::io::Read::read implementations don't usually read
// from their input buffers. But Mockall 0.12-0.13 will try to Debug::fmt the
// buffer, even if there is no failure, triggering an uninitialized data access alert from
// Miri. Initialize the data here just to prevent those Miri alerts.
// This can be removed after upgrading to Mockall 0.14.
dst.fill(0);
Copy link
Member

Choose a reason for hiding this comment

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

Why this workaround is applied only on &MockFile::read() but not on MockFile::read() above (https://github.com/tokio-rs/tokio/pull/7234/files#diff-66254466385396e6861c43b8488cbed96abb2bffae51f0b57dd9cad896c57edfR59) ?
Is it because MockFile::read() is not used in any tests and Miri does not detect the problem ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds right. I'm fine with adding it.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it would be better to update mockall to a newer version and remove this workaround. If an application test needs to assert the contents of the destination buffer then these (trailing?) 0s may lead to confusion

@asomers Does 0.13.1 fixes the issue with the early capturing of the method parameters ?
I could try myself after work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asomers Does 0.13.1 fixes the issue with the early capturing of the method parameters ?
I could try myself after work!

No it does not. But the next release of Mockall will.

Copy link
Member

Choose a reason for hiding this comment

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

#7596 adds the workaround for MockFile too

self.inner_read(dst)
}
}
Expand Down
Loading