Skip to content

Conversation

@asomers
Copy link
Contributor

@asomers asomers commented Apr 3, 2025

This eliminates a duplicate dependency on syn

Motivation

Building mockall-0.11 required tokio to build both syn-1 and syn-2

Solution

Update the mockall dev dependency to the latest version.

@asomers
Copy link
Contributor Author

asomers commented Apr 3, 2025

I don't understand the miri error, even though I can reproduce it locally. It shouldn't be possible for Mockall itself to read uninitialized memory, because Mockall doesn't contain any unsafe code. This will need some more investigation...

@asomers
Copy link
Contributor Author

asomers commented Apr 3, 2025

I mostly understand the problem now. The uninitialized memory is coming from

let buf = unsafe { &mut *(buf as *mut [MaybeUninit<u8>] as *mut [u8]) };
. The failure happens because of a complicated interaction:

  • std::io::Read::read, which specifies that implementors should not attempt to read from the provided buffer,
  • tokio::io::blocking::Buf::read_from, which provides uninitialized memory to the blocking Read::read
  • Mockall 0.12 and later, which attempt to use Debug::fmt on a mock method's arguments if the method's expectations aren't satisfied,
  • The implementation of Debug on slice, which tries to debug every element
  • Miri, which for some reason I don't understand is complaining about Mockall's use of Debug::fmt. I don't understand it, because since these tests don't fail Mockall should never be attempting to print the mock method's arguments.

I don't know why Miri cares about a method that never gets called.

@asomers
Copy link
Contributor Author

asomers commented Apr 3, 2025

Here's a more complete explanation: asomers/mockall#647

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-fs Module: tokio/fs labels Apr 7, 2025
@Darksonn
Copy link
Contributor

Darksonn commented Apr 7, 2025

It is true that implementors of Read are allowed to read from the buffer and that this code triggers UB if that happens. However, in this case Tokio relies on the fact that std::fs::File does not read from the buffer, even if it could.

I guess in this case, the problem is that the type is not std::fs::File, and instead a mockall type that does not behave in the same way.

You can add

#[cfg(test)] 
buf.fill(0);

to silence this error.

This eliminates a duplicate dependency on syn
@asomers
Copy link
Contributor Author

asomers commented Apr 16, 2025

@Darksonn this behavior will be fixed in the next release of Mockall. However, it will require Rust 1.77.0, which is too new for Tokio. So for now I've fixed the Miri failure by zero-initializing the buffer.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks. Regarding the MSRV, we do not require dev-deps to be compatible with our MSRV.

@Darksonn Darksonn merged commit 5490267 into tokio-rs:master Apr 23, 2025
83 checks passed
// 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

martin-g added a commit to martin-g/tokio that referenced this pull request Sep 9, 2025
tokio-rs#7234 did it only for
&MockFile::read().
The same is needed for the owned version of the API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-tokio Area: The main tokio crate M-fs Module: tokio/fs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants