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

Feature: `assert_impl_any!` #19

Closed
nvzqz opened this issue Oct 2, 2019 · 9 comments

Comments

@nvzqz
Copy link
Owner

commented Oct 2, 2019

Asserts a given type implements any of a given set of traits.

@Lej77

This comment has been minimized.

Copy link

commented Oct 2, 2019

I implemented an assert_impl_any macro that mostly works. A blanket trait with a specific method name can cause the macro to fail when it would usually succeed but it can't cause the macro to succeed where it should fail so its not really a problem.

macro_rules! assert_impl_any {
    ($x:ty: $($t:path),+ $(,)?) => {
        const _: fn() = || {
            let previous = {
                struct AssertImplAnyFallback;
                AssertImplAnyFallback
            };

            // Ensures that blanket traits can't impersonate the method.
            struct __ActualAssertImplAnyToken;

            $(
                let previous = {
                    struct __Wrapper<T, N>(core::marker::PhantomData<T>, N);
                    // If the `__static_assert__impl_any_trait` method for this wrapper can't be called then
                    // the compiler will insert a deref and try again. This forwards the compilers next
                    // attempt to the previous wrapper.
                    impl<T, N> core::ops::Deref for __Wrapper<T, N> {
                        type Target = N;
                        fn deref(&self) -> &Self::Target {
                            &self.1
                        }
                    }
                    // This impl has a trait bound on $x so the method can only be called if $x implements $t.
                    impl<T: $t, N> __Wrapper<T, N> {
                        #[allow(non_snake_case)]
                        fn __static_assert__impl_any_trait(&self) -> __ActualAssertImplAnyToken { __ActualAssertImplAnyToken }
                    }
                    __Wrapper::<$x, _>(core::marker::PhantomData, previous)
                };
            )+

            {
                trait AssertImplAnyToken: Sized {}
                impl AssertImplAnyToken for __ActualAssertImplAnyToken {}
                fn assert_impl_any_token<T: AssertImplAnyToken>(_token: T) {}

                // Attempt to find a `__static_assert__impl_any_trait` method that can actually be called.
                // The found method must return a type that implements the sealed `Token` trait, this ensures that blanket trait methods can't cause this macro to compile.
                assert_impl_any_token(previous.__static_assert__impl_any_trait());
            }
        };
    };
}

Playground link (includes some tests)

This code uses a trick that I also made a comment about in snafu issue #88.

@nvzqz nvzqz closed this in 59ac54f Oct 17, 2019
@nvzqz

This comment has been minimized.

Copy link
Owner Author

commented Oct 17, 2019

@Lej77 Thank you so much for this! I wouldn't even have thought of doing it this way. I implemented a simpler version of this based on your implementation (see 59ac54f).

It didn't make sense to me as to why the AssertImplAnyToken trait was necessary so I used the ActualAssertImplAnyToken type directly.

@Lej77

This comment has been minimized.

Copy link

commented Oct 17, 2019

The reason for the AssertImplAnyToken trait was to ensure that a blanket trait that was in scope couldn't cause the macro to actually compile when it shouldn't. The interference_for_fail_case test case on the playground showed how this could be done:

use playground ::assert_impl_any;
trait Interfere<T> {
    #[allow(non_snake_case)]
    fn _static_assertions_impl_any(&self) -> T { loop {} }
}
impl<T, Token> Interfere<Token> for T {}
// These should fail to compile but the above trait might interferes and causes them to actually compile anyway. Fortunately the function must return a special type to compile and that isn't possible.
assert_impl_any!((): From<u8>);
assert_impl_any!((): From<u8>, From<u16>);

If you use the type directly then the users blanket trait can have a generic return type and compile correctly while if type inference only know that the return type must implement a trait then that won't work.

It probably would be better to do something like this so that the token trait isn't in scope for the macro variables (notice that the trait is in a block with a new scope):

{
    trait AssertImplAnyTokenTrait {}
    impl AssertImplAnyTokenTrait for AssertImplAnyToken {}
    fn assert_impl_any_token<T: AssertImplAnyTokenTrait>(_token: T) {}
    assert_impl_any_token(previous._static_assertions_impl_any());
}

I updated my previous comment's suggested code to try and minimize possible name collisions with user provided types and identifiers in the macro.

@nvzqz

This comment has been minimized.

Copy link
Owner Author

commented Oct 18, 2019

Ok I follow as to how it prevents that case from compiling. However, I feel as though this adds a bit too much complexity. I'm leaning towards it not being worth it just to stop people from bypassing this with _static_assertions_impl_any.

In terms of how this could be used in a malicious way outside of the user's crate... I suppose one could add _static_assertions_impl_any! to a commonly used trait with #[doc(hidden)]. But I find that to be extremely unlikely.

I'll add this protection for that reason, but I'm not happy with the added complexity. But, then again, this crate is filled with obscure and complex stuff anyway. 😅

@Lej77

This comment has been minimized.

Copy link

commented Oct 18, 2019

Yeah, I would like it to be simpler and more elegant too but I thought that it was important that the assert couldn't compile when it shouldn't. The scenario I imagined was reading other peoples code and them having intentionally (maliciously?) interfered with an assert so that the reader would assume something untrue.

@Lej77

This comment has been minimized.

Copy link

commented Oct 18, 2019

I saw that you used this macro to implement an assert_impl_one macro. I think that macro can actually be implemented using the same trick as the assert_not_impl_any macro instead:

macro_rules! assert_impl_one {
    ($x:ty: $($t:path),+ $(,)?) => {
        const _: fn() = || {
            // Generic trait.
            trait AmbiguousIfMoreThanOne<A> {
                // Required for actually being able to reference the trait.
                fn some_item() {}
            }

            // Creates multiple scoped `Token` types for each trait `$t`, over
            // which a specialized `AmbiguousIfMoreThanOne<Token>` is implemented
            // for every type that implements `$t`.
            $({
                #[allow(dead_code)]
                struct Token;

                impl<T: ?Sized + $t> AmbiguousIfMoreThanOne<Token> for T {}
            })+

            // If there is only one specialized trait impl, type inference with
            // `_` can be resolved and this can compile. Fails to compile if
            // `$x` implements more than one `AmbiguousIfMoreThanOne<Token>`.
            let _ = <$x as AmbiguousIfMoreThanOne<_>>::some_item;
        };
    };
}

Playground link

@nvzqz

This comment has been minimized.

Copy link
Owner Author

commented Oct 18, 2019

The scenario I imagined was reading other peoples code and them having intentionally (maliciously?) interfered with an assert so that the reader would assume something untrue.

Isn't that just shooting yourself in the foot as the crate author? I can't imagine a sane reason to trick the readers of your code that way. Seeing an unused _static_assertions_impl_any is a big red flag that something weird is up.


Huh, so while that is more obscure than the current implementation of assert_impl_one!, it does seem less complex. It's not self-referential and this sort of thing already exists in the same file. Also, it's not prone to the _static_assertions_impl_any "attack". I think I'll go with this approach instead, thanks!

@Lej77

This comment has been minimized.

Copy link

commented Oct 18, 2019

I imagine that the _static_assertions_impl_any "attack" will probably only happen intentionally (it does seem quite unlikely) and I think the largest reason to do so would be to try and introduce a security vulnerability in some code that is reliant on some invariant that the assert ensures. The reason a crate author would do it to their own crate would be to try and trick the reader that their crate is safe when it really contains a vulnerability that the author could somehow exploit. I don't really know much about security but I assume something like this is possible.

nvzqz added a commit that referenced this issue Oct 18, 2019
Uses a similar trick as `assert_not_impl_any!`. Although this is more
obscure, it feels less complex than the previous version. It's not
self-referential and this sort of thing already exists in the same file.

This implementation was suggested by @Lej77 at:
#19 (comment)
nvzqz added a commit that referenced this issue Oct 18, 2019
An attacker could have a blanket-impl'd trait in scope that has a
`_static_assertions_impl_any` method which interferes with that method
of `Wrapper`. This was pointed out by @Lej77.

See #19.
@nvzqz

This comment has been minimized.

Copy link
Owner Author

commented Oct 18, 2019

Yeah the security concerns are definitely valid, so I added back your protection in 421b6ae.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.