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

Check alignment of raw pointers in debug mode #54915

Closed
RalfJung opened this issue Oct 8, 2018 · 7 comments · Fixed by #98112
Closed

Check alignment of raw pointers in debug mode #54915

RalfJung opened this issue Oct 8, 2018 · 7 comments · Fixed by #98112
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2018

It is UB to *r a raw pointer ("dereference", but this includes &*r) when that pointer is not aligned to its type. I propose that when compiling in debug mode, we insert a check for this to emit a panic when the pointer is not sufficiently aligned. That would essentially be the "big brother" of #52972, and the cousin of #53871.

This would have caught #54908.

However, I guess before someone starts implementing this, we should get @rust-lang/compiler to say that they think this is a good idea. It's in debug mode only, and it's a rather cheap check (as alignment is always a power of two), so I expect the perf impact to be acceptable.

@RalfJung RalfJung added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 8, 2018
@leonardo-m
Copy link

leonardo-m commented Oct 9, 2018

See also Zig, that exposes such alignment information in the type system itself:
https://andrewkelley.me/post/unsafe-zig-safer-than-unsafe-rust.html

Using syntax like:

var array align(@alignOf(Foo)) = []u8{1} ** 1024;

The run-time check perf impact is probably acceptable because currently debug builds are often 1000%+ slower.

I suggest to add a switch like -Z force-overflow-checks that could be switched on in "efficient experiment" release builds too if desired, because sometimes debug builds are so slow you can't use them well to test some code.

More generally, what other basic invariants like that could rustc verify automatically (in debug builds)?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 9, 2018

I suggest to add a switch like -Z force-overflow-checks that could be switched on in "efficient experiment" release builds too if desired, because sometimes debug builds are so slow you can't use them well to test some code.

Good point. We have something similar for integer overflow, don't we?

More generally, what other basic invariants like that could rustc verify automatically (in debug builds)?

Tons of them, but which have a good perf-benefit trade-off? Also note that this wouldn't reliably verify the alignment invariant, one can still mem::transmute or use a union or cast pointer types around to get an underaligned (or NULL) reference. It's not a perfect check (unlike what we can do e.g. in miri), but it's useful.

Some other things coming to my mind are:

  • Instead of triggering UB when having an uninhabited local variable, trigger a llvm.trap.
  • When doing a match on an enum or bool, verify that the discriminant is valid.
  • Go over the intrinsics and see which could check some of their preconditions -- things like unchecked_div or unchecked_shl. Would it be better to check this is the stable wrappers around the intrinsics, or change the code generated by the intrinsics?
  • Go over the unsafe functions in libstd and see which of these could get debug_assert! checking (some of) their preconditions. Things coming to my mind are from_utf8_unchecked and get_unchecked. Here's an incomplete list.

For the last (or the last two) to be viable, however, we need a libstd compiled with debug assertion to be used when compiling programs in debug mode.

Everything else I can think of involves some kind of type-based recursion on e.g. the return value of mem::transmute, which is much more work and much less useful (as mentioned above, there are many other ways to transmute data).


Finally, we'd probably have to do some communication work to get people to run their test suites in debug mode, so that this actually reaches all the projects out there. Is there any way to get debug assertions, but also get optimizations, so that this doesn't make everything quite so much slower? It does seem useful to me to have code with extra checks optimized; that means the compiler can assume fewer things (less UB) but it can still e.g. optimize away the checks if it can prove they always succeed, and it can do all the other optimizations (inlining and whatnot).

@arielb1
Copy link
Contributor

arielb1 commented Oct 9, 2018

Is there any way to get debug assertions, but also get optimizations, so that this doesn't make everything quite so much slower?

Sure enough. -O -C debug-assertions=on. Not sure how to do it via cargo.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 9, 2018

Ah, nice! And that will also affect integer overflow checks?

@arielb1
Copy link
Contributor

arielb1 commented Oct 13, 2018

Ah, nice! And that will also affect integer overflow checks?

Integer overflow checks will be on (they are controlled by debug-assertions, not by -O).

@memoryruins
Copy link
Contributor

memoryruins commented Oct 14, 2018

With Cargo, the flags are placed in [profile.*] sections. The following are two of the defaults for dev builds

# The development profile, used for `cargo build`.
[profile.dev]
opt-level = 0      # controls the `--opt-level` the compiler builds with.
                   # 0-1 is good for debugging. 2 is well-optimized. Max is 3.
                   # 's' attempts to reduce size, 'z' reduces size even more.
debug-assertions = true # controls whether debug assertions are enabled
                   # (e.g. debug_assert!() and arithmetic overflow checks)

Finally, we'd probably have to do some communication work to get people to run their test suites in debug mode, so that this actually reaches all the projects out there

Fortunately, the default travis-ci rust set-up runs cargo build --verbose and cargo test --verbose, and the samples for travis and gitlab in the Cargo book do similarly. Both build and test use [profile.dev] by default, so it would definitely reach many out there without even mentioning, at least for unoptimized debug builds and tests.

@RalfJung
Copy link
Member Author

Cc #51713

@bors bors closed this as completed in 22a7a19 Mar 31, 2023
oli-obk pushed a commit to oli-obk/miri that referenced this issue Apr 4, 2023
Insert alignment checks for pointer dereferences when debug assertions are enabled

Closes rust-lang/rust#54915

- [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit)
- [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue!
- [x] Implement a more helpful panic message like slice bounds checking.

r? `@oli-obk`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Apr 6, 2023
Insert alignment checks for pointer dereferences when debug assertions are enabled

Closes rust-lang/rust#54915

- [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit)
- [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue!
- [x] Implement a more helpful panic message like slice bounds checking.

r? `@oli-obk`
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Apr 9, 2023
Insert alignment checks for pointer dereferences when debug assertions are enabled

Closes rust-lang/rust#54915

- [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit)
- [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue!
- [x] Implement a more helpful panic message like slice bounds checking.

r? `@oli-obk`
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jun 1, 2023
Insert alignment checks for pointer dereferences when debug assertions are enabled

Closes rust-lang/rust#54915

- [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit)
- [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue!
- [x] Implement a more helpful panic message like slice bounds checking.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants