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

[Merged by Bors] - Ensure Ptr/PtrMut/OwningPtr are aligned when casting in debug builds #7117

Closed
wants to merge 14 commits into from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Jan 7, 2023

Objective

Improve safety testing when using bevy_ptr types. This is a follow-up to #7113.

Solution

Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.


Changelog

Added: Ptr::deref will now panic in debug builds if the pointer is not aligned.
Added: PtrMut::deref_mut will now panic in debug builds if the pointer is not aligned.
Added: OwningPtr::read will now panic in debug builds if the pointer is not aligned.
Added: OwningPtr::drop_as will now panic in debug builds if the pointer is not aligned.

@james7132 james7132 added the P-Unsound A bug that results in undefined compiler behavior label Jan 7, 2023
crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

LGTM. I believe the CI failure is a pre-existing bug.

crates/bevy_ptr/src/lib.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Jan 8, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 8, 2023

the CI failure is caused by the fact that BlobVec::new does not create an aligned pointer for non-zsts:

            let mut blob_vec = BlobVec {
                data: NonNull::dangling(),
                capacity: 0,
                len: 0,
                item_layout,
                drop,
            };

it should be data: bevy_ptr::dangling_with_align(align) like we do for ZSTs (see also #6618 which fixed a similar bug for zsts)

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 9, 2023

This PR doesn't add a safety invariant to the new function's of Ptr/PtrMut/OwningPtr requiring the pointer to have correct alignment for the pointee. Although it was pointed out that it was useful to use these types for commands which are not aligned 🤔 I wonder if it would be worth adding a defaulted param to these for whether they're aligned or not: struct Ptr<'a, A = Aligned> {

(alternatively we could treat them normal raw pointers and not require alignment, instead opting to make read and friends unsafe with an invariant that they are correctly aligned... but I do not like this much as "borrow but without the pointee type known" is a nice mental model that would be a shame to lose)

@james7132
Copy link
Member Author

I guess my question is how do you enforce alignment while type erased?

We could provide safe conversions from normal borrows, mutable or not. However, we don't even have that in the ECS where there may not be a type at all to work with.

Only other option is to explicitly provide the layout or alignment to the constructor and at that point it feels sort of redundant/clunky. Like asking a student to grade their own homework.

We already support unaligned reads on the types so IMO treating these as "lifetimed type erased pointers" over "type erased borrows" makes a lot more sense.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 9, 2023

You enforce it with a safety invariant // SAFETY: The pointer must be sufficiently aligned for the erased pointee type. You would still assert it in the read/deref methods like you're doing in this PR.

edit: If we aren't enforcing it on construction then we need to have safety invariants on read/deref etc that we are sufficiently aligned even though in practically every use case this is something we should know on construction of the Ptr so it just seems better to me to do it on construction

@james7132
Copy link
Member Author

I think #7113 adds exactly that. Perhaps we should merge that first?

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 9, 2023

I was not aware we already had PR open adding those invariants thats good to know before its merged...

@alice-i-cecile
Copy link
Member

I want #7151 merged before this.

bors bot pushed a commit that referenced this pull request Jan 10, 2023
…7151)

# Objective

The types in the `bevy_ptr` accidentally did not document anything relating to alignment. This is unsound as many methods rely on the pointer being correctly aligned. 

## Solution

This PR introduces new safety invariants on the `$ptr::new`, `$ptr::byte_offset` and `$ptr::byte_add` methods requiring them to keep the pointer aligned. This is consistent with the documentation of these pointer types which document them as being "type erased borrows".

As it was pointed out (by @JoJoJet in #7117) that working with unaligned pointers can be useful (for example our commands abstraction which does not try to align anything properly, see #7039) this PR also introduces a default type parameter to all the pointer types that specifies whether it has alignment requirements or not. I could not find any code in `bevy_ecs` that would need unaligned pointers right now so this is going unused.

---

## Changelog

- Correctly document alignment requirements on `bevy_ptr` types.
- Support variants of `bevy_ptr` types that do not require being correctly aligned for the pointee type.

## Migration Guide

- Safety invariants on `bevy_ptr` types' `new` `byte_add` and `byte_offset` methods have been changed. All callers should re-audit for soundness.
@james7132
Copy link
Member Author

With #7151 merged, @BoxyUwU @JoJoJet @alice-i-cecile, this PR should be ready.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 11, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 11, 2023
…7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to #7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
@bors bors bot changed the title Ensure Ptr/PtrMut/OwningPtr are aligned when casting in debug builds [Merged by Bors] - Ensure Ptr/PtrMut/OwningPtr are aligned when casting in debug builds Jan 11, 2023
@bors bors bot closed this Jan 11, 2023
@james7132 james7132 deleted the debug-ptr-align-assert branch January 20, 2023 01:12
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…evyengine#7151)

# Objective

The types in the `bevy_ptr` accidentally did not document anything relating to alignment. This is unsound as many methods rely on the pointer being correctly aligned. 

## Solution

This PR introduces new safety invariants on the `$ptr::new`, `$ptr::byte_offset` and `$ptr::byte_add` methods requiring them to keep the pointer aligned. This is consistent with the documentation of these pointer types which document them as being "type erased borrows".

As it was pointed out (by @JoJoJet in bevyengine#7117) that working with unaligned pointers can be useful (for example our commands abstraction which does not try to align anything properly, see bevyengine#7039) this PR also introduces a default type parameter to all the pointer types that specifies whether it has alignment requirements or not. I could not find any code in `bevy_ecs` that would need unaligned pointers right now so this is going unused.

---

## Changelog

- Correctly document alignment requirements on `bevy_ptr` types.
- Support variants of `bevy_ptr` types that do not require being correctly aligned for the pointee type.

## Migration Guide

- Safety invariants on `bevy_ptr` types' `new` `byte_add` and `byte_offset` methods have been changed. All callers should re-audit for soundness.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…evyengine#7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to bevyengine#7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#7151)

# Objective

The types in the `bevy_ptr` accidentally did not document anything relating to alignment. This is unsound as many methods rely on the pointer being correctly aligned. 

## Solution

This PR introduces new safety invariants on the `$ptr::new`, `$ptr::byte_offset` and `$ptr::byte_add` methods requiring them to keep the pointer aligned. This is consistent with the documentation of these pointer types which document them as being "type erased borrows".

As it was pointed out (by @JoJoJet in bevyengine#7117) that working with unaligned pointers can be useful (for example our commands abstraction which does not try to align anything properly, see bevyengine#7039) this PR also introduces a default type parameter to all the pointer types that specifies whether it has alignment requirements or not. I could not find any code in `bevy_ecs` that would need unaligned pointers right now so this is going unused.

---

## Changelog

- Correctly document alignment requirements on `bevy_ptr` types.
- Support variants of `bevy_ptr` types that do not require being correctly aligned for the pointee type.

## Migration Guide

- Safety invariants on `bevy_ptr` types' `new` `byte_add` and `byte_offset` methods have been changed. All callers should re-audit for soundness.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#7117)

# Objective
Improve safety testing when using `bevy_ptr` types. This is a follow-up to bevyengine#7113.

## Solution
Add a debug-only assertion that pointers are aligned when casting to a concrete type. This should very quickly catch any unsoundness from unaligned pointers, even without miri. However, this can have a large negative perf impact on debug builds.

---

## Changelog
Added: `Ptr::deref` will now panic in debug builds if the pointer is not aligned.
Added: `PtrMut::deref_mut` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::read` will now panic in debug builds if the pointer is not aligned.
Added: `OwningPtr::drop_as` will now panic in debug builds if the pointer is not aligned.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help! P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants