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

run some tests with Miri #2795

Closed
wants to merge 4 commits into from
Closed

run some tests with Miri #2795

wants to merge 4 commits into from

Conversation

RalfJung
Copy link
Contributor

@RalfJung RalfJung commented Mar 27, 2022

We have to limit the set of features we test: --feature full does not work since it fires up the tokio runtime, which calls epoll_create1 and that is not supported by Miri.

Fixes #2794

@RalfJung

This comment was marked as resolved.

@RalfJung

This comment was marked as resolved.

@RalfJung
Copy link
Contributor Author

RalfJung commented Mar 27, 2022

I can confirm that with seanmonstar/httparse#113, running the tests with --features http1,http2,client,server in Miri works. :) And that would have caught the problem fixed by #2545.

@RalfJung
Copy link
Contributor Author

Does httparse 1.7.0 contain seanmonstar/httparse#113? If yes, then this PR could move forward once hyper depends on the newer httparse. :D

@seanmonstar
Copy link
Member

Oh, nice! I was wondering, the point of running all the tests through Miri is to check for any UB in the stack?

@RalfJung
Copy link
Contributor Author

Yes. Specifically this would have caught the UB fixed by #2545 if it had been added a year ago.

@RalfJung
Copy link
Contributor Author

It looks like the Miri CI job here has been rerun a few days ago (not quite sure what triggered that), has picked up the new httparse, and has subsequently gotten a green checkmark. :)

@RalfJung RalfJung force-pushed the miri branch 2 times, most recently from 94e9cf9 to f974123 Compare April 17, 2022 13:53
@RalfJung
Copy link
Contributor Author

RalfJung commented Apr 17, 2022

Ah, bummer, with strict provenance tests start to fail:

error: Undefined Behavior: dereferencing pointer failed: 0x6b7cbe is not a valid pointer
    --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:141:9
     |
141  |         &mut *ptr::slice_from_raw_parts_mut(data, len)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ dereferencing pointer failed: 0x6b7cbe is not a valid pointer
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
             
     = note: inside `std::slice::from_raw_parts_mut::<u8>` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:141:9
     = note: inside `bytes::bytes::rebuild_boxed_slice` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.1.0/src/bytes.rs:938:19
     = note: inside `bytes::bytes::shallow_clone_vec` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.1.0/src/bytes.rs:1009:15
     = note: inside `bytes::bytes::promotable_even_clone` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.1.0/src/bytes.rs:890:9
     = note: inside `<bytes::Bytes as std::clone::Clone>::clone` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.1.0/src/bytes.rs:522:18
     = note: inside `bytes::Bytes::split_to` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.1.0/src/bytes.rs:417:23
     = note: inside `http::uri::parse_full` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/http-0.2.6/src/uri/mod.rs:807:21
     = note: inside `http::Uri::from_shared` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/http-0.2.6/src/uri/mod.rs:307:9
     = note: inside `<http::Uri as std::convert::TryFrom<&[u8]>>::try_from` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/http-0.2.6/src/uri/mod.rs:681:9
     = note: inside `<http::Uri as std::str::FromStr>::from_str` at /home/r/.cargo/registry/src/github.com-1ecc6299db9ec823/http-0.2.6/src/uri/mod.rs:866:9
     = note: inside `core::str::<impl str>::parse::<http::Uri>` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/str/mod.rs:2344:9
note: inside `client::client::unit_tests::set_relative_uri_with_implicit_path` at src/client/client.rs:1371:23
    --> src/client/client.rs:1371:23
     |
1371 |         let mut uri = "http://hyper.rs".parse().unwrap();
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^

Looks like there are int-to-ptr casts somewhere. Probably in bytes.

Checking without strict provenance is still valuable though. :)

@seanmonstar
Copy link
Member

Yea, bytes was written in the dark ages, when pointers were integers and blindfolds were typical headgear :)

So, normally I wouldn't have a contributor care about the commit format, since I can clean it up in a squash. But in this case, the commits really should be separate. So, either, could you rename the commits as docs/COMMITS.md mentions, or I can do a manual merge from the command line when I'm back at a computer.

@RalfJung
Copy link
Contributor Author

bytes master branch is actually strict provenance compatible: tokio-rs/bytes#542. ;) So in due time the flag can probably also be enabled here.

@RalfJung
Copy link
Contributor Author

How separate do you want the commits to be? All 4 individually like now?

I can look into the commit format thing later -- or feel free to do it if you get to it first.

@RalfJung
Copy link
Contributor Author

Looking at the commits documentation, I think I best leave that part to you -- figuring out the "type" and "scope" of each of these changes seems rather subjective and certainly requires more familiarity with this crate than I have.

@seanmonstar
Copy link
Member

Alright, I manually merged this to master in 8834d5a and e1138d7. Thanks!

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.

'cargo test --features http2' fails with many "associated function is never used"
2 participants