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

Unsoundness bug #1

Closed
oddcoder opened this issue Aug 19, 2024 · 0 comments
Closed

Unsoundness bug #1

oddcoder opened this issue Aug 19, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@oddcoder
Copy link
Owner

Right now you could have something like this:

#[derive(Discriminant)]
#[bad_macro]
#[repr(i64)]
enum Foo {
    A = 0
}

with the #[derive(Discriminant)] expanded it would become:

#[bad_macro]
#[repr(i64)]
enum Foo {
    A = 0
}

unsafe impl Discriminant for Foo { ... }

and with the #[bad_macro] expanded:

enum Foo {
    A = 0
}

unsafe impl Discriminant for Foo { ... }

Notice that #[bad_macro] removed the #[repr(i64)], breaking the requirements of your unsafe impl.

With an attribute macro you would instead have something like:

#[derive_discriminant)]
#[bad_macro]
#[repr(i64)]
enum Foo {
    A = 0
}

where expanding #[derive_discriminant] would give you this (notice the #[repr(i64)] moved on top!):

#[repr(i64)]
#[bad_macro]
enum Foo {
    A = 0
}

unsafe impl Discriminant for Foo { ... }

In this case #[bad_macro] would not be able to remove the #[repr(i64)] when expanding because it has been moved before it. Note that a derive macro can't do this because it can't modify the item it's applied on, it can only generate more tokens.

However with eager macro expansion (nightly-only right now, and likely not close to stabilization) a bad macro could do this:

#[bad_macro]
#[derive_discriminant)]
#[repr(i64)]
enum Foo {
    A = 0
}

With #[bad_macro] first eagerly expanding the macros in the inner item, obtaining:

#[repr(i64)]
enum Foo {
    A = 0
}

unsafe impl Discriminant for Foo { ... }

And then removing the #[repr(i64)], obtaining:

enum Foo {
    A = 0
}

unsafe impl Discriminant for Foo { ... }

which is again unsound.

Credits: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/feedback.20request.3A.20safe.20discriminant.20extraction.20from.20enums/near/463033438

@oddcoder oddcoder added the bug Something isn't working label Aug 19, 2024
oddcoder added a commit that referenced this issue Aug 19, 2024
This macro removes `#[repr(_)]` from an enum, this is useful for testing
the bug described in #1. `#[remove_repr]` is only available behind the
feature flag test-utils.

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
@oddcoder oddcoder mentioned this issue Aug 19, 2024
oddcoder added a commit that referenced this issue Aug 19, 2024
This patch checks if there is any top level attribute macro expansion,
and if there it, we report it as an error.

fixes: #1

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
oddcoder added a commit that referenced this issue Aug 19, 2024
This patch checks if there is any top level attribute macro expansion,
and if there it, we report it as an error.

fixes: #1

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
oddcoder added a commit that referenced this issue Aug 19, 2024
This macro removes `#[repr(_)]` from an enum, this is useful for testing
the bug described in #1. `#[remove_repr]` is only available behind the
feature flag test-utils.

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
oddcoder added a commit that referenced this issue Aug 19, 2024
This patch checks if there is any top level attribute macro expansion,
and if there it, we report it as an error.

fixes: #1

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
oddcoder added a commit that referenced this issue Aug 19, 2024
It would have been security risk similar to #1 if that was accepted,
but it is not.

closes #5

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
oddcoder added a commit that referenced this issue Aug 19, 2024
It would have been security risk similar to #1 if that was accepted,
but it is not.

closes #5

Signed-off-by: Ahmed Abdelraoof <ahmed.abdelraoof@huawei.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant