Skip to content

ci: run miri + adjustments for miri #128

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

Merged
merged 2 commits into from
Jun 23, 2023
Merged

ci: run miri + adjustments for miri #128

merged 2 commits into from
Jun 23, 2023

Conversation

phip1611
Copy link
Member

Closes #117

@phip1611 phip1611 force-pushed the ci-miri branch 2 times, most recently from 9e479ad to e122ad5 Compare March 18, 2023 12:42
@phip1611 phip1611 force-pushed the ci-miri branch 5 times, most recently from ee2f968 to bb84406 Compare March 26, 2023 19:10
@phip1611
Copy link
Member Author

phip1611 commented Mar 26, 2023

Could I ask you @nicholasbishop please for help? You do have more experience with miri than I have, I think. I don't quite understand why miri fails and if this is valid or how I should either fix or ignore it. Do you have some advice?

You can find a failed run here: https://github.com/rust-osdev/multiboot2/actions/runs/4526183512/jobs/7971182214

@nicholasbishop
Copy link
Member

I think the error in multiboot2/src/boot_loader_name.rs:34 is due to rust-lang/unsafe-code-guidelines#256. I haven't read through that full thread, so there are probably more ways of handling this, but what I did in uefi-rs is convert the header type to a DST. So the BootLoaderNameTag.string field would become a [u8]. See https://github.com/rust-osdev/uefi-rs/blob/main/uefi/src/proto/device_path/mod.rs#L126 for one example of this.

@phip1611 phip1611 mentioned this pull request Mar 27, 2023
@phip1611 phip1611 changed the title ci: run miri DRAFT: ci: run miri + adjustments for miri Jun 21, 2023
@phip1611 phip1611 assigned phip1611 and unassigned phip1611 Jun 21, 2023
@phip1611 phip1611 marked this pull request as draft June 21, 2023 14:31
@phip1611
Copy link
Member Author

phip1611 commented Jun 21, 2023

At this state, miri is somewhat useful. All tests that use the cast_tag() function (transitively) or the TagIter, need the #[cfg_attr(miri, ignore)] attribute.. I think that we are memory safe and miri just can't proof it.

A possible solution might be to model the whole parsing logic differently:

  1. get pointer to mbi
  2. find end
  3. create a slice
  4. operate on that slice. Then, all mem options are part of the "borrowed memory stack", according to miri

I need to think, if this should be merged in this state. I do not like the need for the (feature incomplete) AVec abstraction (aligned-vec).

@phip1611
Copy link
Member Author

Hm. In #155 (comment) I could at least address some of the issues.

@phip1611 phip1611 changed the title DRAFT: ci: run miri + adjustments for miri ci: run miri + adjustments for miri Jun 23, 2023
@phip1611 phip1611 self-assigned this Jun 23, 2023
@phip1611 phip1611 marked this pull request as ready for review June 23, 2023 11:52
I decided so as they only make things more complicated for miri. As we have
mature builder and parser structs guaranteeing alignment, we do not have to
create additional confusion. These align(8) markers do not have value-add
here besides being informational.

They are only useful if no crazy pointer magic is involved and Rust constructs
these types "in the Rust-typical way".
@phip1611
Copy link
Member Author

phip1611 commented Jun 23, 2023

This MR now introduces basic miri support. Most tests are skipped by miri. There are two major issues:

  • Each test that uses the cast_tag (transitively) fails, as miri can't guarantee that the memory is valid, unfortunately
  • when something like struct_as_bytes() ... cast is done, there are alignment issues as the returned Vec is of course only aligned to a one byte boundary... so far, in tests this never was a problem, luckily. At least on x86.

However, some things now are tracked by miri. For example if BoxedDst works properly.

@phip1611 phip1611 merged commit 6ea885a into main Jun 23, 2023
@phip1611 phip1611 deleted the ci-miri branch June 23, 2023 12:04
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.

CI: Run Tests with Miri to Catch Memory Issues
2 participants