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

add option to disable "incorrect layout on deallocation" error check #1873

Closed
kazuk opened this issue Aug 19, 2021 · 7 comments
Closed

add option to disable "incorrect layout on deallocation" error check #1873

kazuk opened this issue Aug 19, 2021 · 7 comments
Labels
A-allocator Area: related to memory allocation C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available

Comments

@kazuk
Copy link

kazuk commented Aug 19, 2021

I testing code with MIRI,

tested code has incorrect layout on deallocation error.

https://doc.rust-lang.org/std/vec/struct.Vec.html#method.from_raw_parts

Violating these may cause problems like corrupting the allocator’s internal data structures. For example it is not safe to build a Vec from a pointer to a C char array with length size_t. It’s also not safe to build one from a Vec and its length, because the allocator cares about the alignment, and these two types have different alignments. The buffer was allocated with alignment 2 (for u16), but after turning it into a Vec it’ll be deallocated with alignment 1.

If Allocator implementation is safe against for alignment mismatches to alloc-dealloc, I can ignore the "incorrect layout on deallocation" error.

I think default allocator is SAFE , I decide to ignore this.

I try miri-disable-alignment-check MIRIFLAG

MIRIFLAGS="-Zmiri-disable-alignment-check" cargo +nightly miri test

But reports incorrect layout on deallocation again.

Can I ignore this error?

@bjorn3
Copy link
Member

bjorn3 commented Aug 19, 2021

I think default allocator is SAFE , I decide to ignore this.

Maybe it is right now, but there is no guarantee. In addition there is no guarantee that a user of your crate or even a dependency doesn't override the global allocator with one that requires the alignment to match.

Can I ignore this error?

I don't think miri has an option for this.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2021

Miri is just following the official documentation. The rationale is in https://doc.rust-lang.org/std/alloc/trait.Allocator.html#memory-fitting and indeed does not care if a specific allocator supports having a different alignment at allocation vs deallocation.

Unless something changes in the official Allocator specification, miri will not change its behaviour.

Furthermore, I personally don't think we should add an option to ignore this error, as it explicitly goes against documented behaviour, unlike the other things where we have opt-outs for, which are for things that haven't been decided on yet.

@RalfJung
Copy link
Member

RalfJung commented Aug 19, 2021

Can I ignore this error?

It is certainly incorrect to ignore this error, unless you explicitly are setting your own allocator that's fine with bad layout information on deallocation (and if you do, the proper way to make this work is #1207).

However, we could in theory have a flag that lets you ignore the error anyway. That would basically mean Miri would deliberately ignore some bugs in your program.

Furthermore, I personally don't think we should add an option to ignore this error, as it explicitly goes against documented behaviour, unlike the other things where we have opt-outs for, which are for things that haven't been decided on yet.

No strong opinion here, but note that we do have -Zmiri-disable-alignment-check -- there is nothing undecided here.

@kazuk
Copy link
Author

kazuk commented Aug 20, 2021

Maybe it is right now, but there is no guarantee.

Yes, I know no guarantee. I report bug to maintainer.
ignore as "Hey miri, I know this is a bug. please find next or other one."

I don't want to add #[cfg_attr(miri, ignore)] to my test as much as possible.

That would basically mean Miri would deliberately ignore some bugs in your program.

Yes.

I hope to markup code block as "known bug here" and feedback to maintainer.

safe.and.correct.code();
#[cfg_attr(miri, known_bug_missmatch_alignment_on_alloc_dealloc)]
{
    buggy_code();
}
safe.and.correct.code();

off-topic:

Can I build MIRI? cargo build failed on flesh clone.
where to find build instruction?

   Compiling miri v0.1.0 (/home/kazuk/miri)
error[E0463]: can't find crate for `rustc_apfloat`
  --> src/lib.rs:10:1
   |
10 | extern crate rustc_apfloat;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

@oli-obk
Copy link
Contributor

oli-obk commented Aug 20, 2021

For building miri yourself, you can follow the contributor docs: https://github.com/rust-lang/miri/blob/master/CONTRIBUTING.md

@RalfJung
Copy link
Member

I hope to markup code block as "known bug here" and feedback to maintainer.

That sounds like #788. This is non-trivial though since sometimes one wants the allow to apply for nested function calls, and sometimes not.

@RalfJung RalfJung changed the title howto question: disable "incorrect layout on deallocation" error check add option to disable "incorrect layout on deallocation" error check Dec 16, 2021
@RalfJung RalfJung added A-interpreter Area: affects the core interpreter C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Dec 16, 2021
@RalfJung RalfJung added E-good-first-issue A good way to start contributing, mentoring is available A-allocator Area: related to memory allocation and removed A-interpreter Area: affects the core interpreter labels Nov 22, 2022
@RalfJung
Copy link
Member

We've generally more been moving in the direction of having fewer such unsound flags (e.g. disabling the ABI checks is on its way out), so I don't think we'll want a flag that disables this check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocator Area: related to memory allocation C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available
Projects
None yet
Development

No branches or pull requests

4 participants