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 container_of #21

Closed
wants to merge 7 commits into from
Closed

Add container_of #21

wants to merge 7 commits into from

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Aug 16, 2019

This adds container_of, an unsafe macro which allows obtaining a raw pointer to the containing struct from a pointer to one of its fields.

I also fixed the other macros to take in a path instead of a tt for the struct name, which allows complex path such as my_mod::Foo.

@Gilnaa
Copy link
Owner

Gilnaa commented Aug 16, 2019

Thank you for your contribution! :)
Is it possible to make sure statically that ptr is a pointer to the same type as field is?

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 16, 2019

I don't think that this is possible since in the macro we do not know the type of the field. However I am open to suggestions!

@Gilnaa Gilnaa requested a review from RalfJung August 16, 2019 09:10
@Gilnaa
Copy link
Owner

Gilnaa commented Aug 16, 2019

You could add this semi-ugly hack.
Haven't tested it with a mut-ptr as input, but it should work

macro_rules! container_of {
    ($ptr:expr, $container:path, $field:tt) => {{
        // The following code segment makes sure that $ptr is
        // indeed a pointer to the same type as $field.
        // We do not have a way to detect the type of $field or $ptr,
        // so instead we create a mut-binding to the field and then re-bind
        // it to $ptr.
        //
        // If they are of differing types, the compiler will complain.
        _memoffset__let_base_ptr!(base_ptr, $container);
        #[allow(unused_unsafe)] // for when the macro is used in an unsafe block
        let mut _field_ptr = unsafe { &(*base_ptr).$field as *const _ };
        _field_ptr = $ptr;

        ($ptr as *const _ as *const u8).offset(-(offset_of!($container, $field) as isize))
            as *mut $container
    }};
}

@Gilnaa
Copy link
Owner

Gilnaa commented Aug 16, 2019

It's probably wise to add compile-fail tests as well

src/container_of.rs Outdated Show resolved Hide resolved
src/container_of.rs Outdated Show resolved Hide resolved
src/container_of.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Collaborator

It's probably wise to add compile-fail tests as well

Yeah, I heard doctests can do that?

We don't have any compile-fail tests yet, that's on my to-do list, so I won't block this PR on that. Opened an issue instead: #23

src/container_of.rs Outdated Show resolved Hide resolved
src/container_of.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Collaborator

RalfJung commented Aug 16, 2019

@Amanieu could you rebase onto master so that we can see what Miri thinks of this?

The more I think about this, the less Stacked Borrows likes this. You cannot use a raw pointer created for a field to access anything but that field. Creating a raw pointer from a reference "remembers" which "permissions" this reference had, and it only has permission to access the stuff it points to (its "span", defined by size_of_val), not what sits next to it. Also see rust-lang/unsafe-code-guidelines#134.

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 16, 2019

I ran Miri locally and it seems happy.

@RalfJung
Copy link
Collaborator

Right, but the test is also not using the resulting pointer. Something like this is likely UB according to Stacked Borrows:

struct Pair { f1: u16, f2: u16 };

let p = Pair { f1: 2, f2: 3 };
let c = container_of!(&p.f1, Pair, f1);
let _val = c.f2;

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 16, 2019

You're right, this does indeed cause a Miri failure:

error[E0080]: Miri evaluation error: no item granting read access to tag <untagged> found in borrow stack
   --> src/container_of.rs:126:24
    |
126 |             let _val = (*c).f2;
    |                        ^^^^^^^ Miri evaluation error: no item granting read access to tag <untagged> found in borrow stack

I'm not really sure what we can do about this. In C this isn't an issue because a pointer anywhere in the middle of an object (i.e an allocation) still allows you to access any byte in that object as long as you make the appropriate casts to satisfy TBAA.

@RalfJung
Copy link
Collaborator

This is indeed somewhat of a unique Rust issue with its rules for references having a "span" and only being useful inside that span. Though C also has lots of rules restricting address arithmetic to work only within one "subobject", so I am not sure I agree with you that this is okay in C. I would agree though that everyone does it in C. ;)

On the other hand, I think this principle is very fundamentally necessary to make Stacked Borrows any use. If uniqueness of mutable references could be legitimately violated by using arithmetic from a "neighboring" field, we might as well not bother with noalias. Maybe if/when we find a solution for rust-lang/unsafe-code-guidelines#133, that can also help here, but I cannot foresee the effect this has on our ability to reason about what unknown code can do.

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 17, 2019

On the other hand, I think this principle is very fundamentally necessary to make Stacked Borrows any use. If uniqueness of mutable references could be legitimately violated by using arithmetic from a "neighboring" field, we might as well not bother with noalias. Maybe if/when we find a solution for rust-lang/unsafe-code-guidelines#133, that can also help here, but I cannot foresee the effect this has on our ability to reason about what unknown code can do.

I think it might be possible to make something workable along the lines of: a reference to a field has weak rights to access the whole object, but those rights are revoked when a mutable reference is performed on one of the other fields.

@RalfJung
Copy link
Collaborator

RalfJung commented Aug 17, 2019

Yeah, maybe -- the referenced issue would be the place to discuss that.

@RalfJung
Copy link
Collaborator

@Amanieu could you factor the macro kind changes (tt -> type) into a separate PR? Those are definitely and independently useful.

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 28, 2019

Done

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 29, 2019

Interestingly, if I add &p.f2 as *const _; (i.e. a statement that does nothing) before container_of then miri stops complaining.:

    #[test]
    fn miri() {
        struct Pair {
            f1: u16,
            f2: u16,
        };
        unsafe {
            let p = Pair { f1: 2, f2: 3 };
            &p.f2 as *const _;
            let c = container_of!(&p.f1, Pair, f1);
            let _val = (*c).f2;
        }
    }

@RalfJung
Copy link
Collaborator

Yeah that's because Miri currently makes no attempt to track raw pointers. But to match what LLVM does, we will have to do more precise tracking eventually, and then this will not work any more.

It will still work if you do enough int-ptr-casts to kill provenance. At that point, what LLVM does is just entirely unknown.

@RalfJung
Copy link
Collaborator

&p.f2 as *const _; (i.e. a statement that does nothing)

This thing does a lot! It creates a raw pointer, grants access to all raw pointers to access this memory, and then the pointer is thrown away.
It's all raw pointers because we currently do not distinguish raw pointers (that's what I mean by "we do not track them").

@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 29, 2019

Would obtaining the field pointer using offset_of solve this issue "properly"?

    #[test]
    fn miri() {
        struct Pair {
            f1: u16,
            f2: u16,
        };
        unsafe {
            let p = Pair { f1: 2, f2: 3 };
            let f = (&p as *const _ as *const u8).add(offset_of!(Pair, f1)) as *const u16;
            let c = container_of!(f, Pair, f1);
            let _val = (*c).f2;
        }
    }

@RalfJung
Copy link
Collaborator

RalfJung commented Aug 29, 2019

The key is to get the pointer provenance right: the pointer to the entire struct (returned by container_of!) must be derived from a pointer that is actually allowed to access that entire struct.

Raw pointer operations to not affect the permissions a pointer has, so the moment you are casting a reference to a raw pointer, you are deciding what is allowed to be done with all raw pointers ever created from this one.

In your example, I think &p as *const _ is the only ref-to-raw cast. That is a cast at type &Pair, so the resulting raw pointer (and all its descendants) is allowed to read from the entire pair.

@Gilnaa
Copy link
Owner

Gilnaa commented Aug 4, 2020

@RalfJung What do we need in order to close/merge this?

@RalfJung
Copy link
Collaborator

RalfJung commented Aug 5, 2020

I'd say let's close this PR due to inactivity. I opened an issue for this at rust-lang/unsafe-code-guidelines#243 for the general problem. I am not sure if @Amanieu still considers the macro useful given these heavy limitations.

@Amanieu, I am okay with having such a macro in principle, so feel free to reopen if you still think the macro is useful. But the docs need to be very clear about the fact that provenance (aka the aliasing model) is a real problem here -- and without rust-lang/rust#73394, it is basically impossible to use the macro correctly.

@RalfJung RalfJung closed this Aug 5, 2020
@Amanieu
Copy link
Contributor Author

Amanieu commented Aug 5, 2020

I am actively using this macro in intrusive-collections. I will continue maintaining my implementation there, but I feel it would be better to have a properly reviewed implementation in memoffset.

My thought was that we could modify raw_field to use offset_of internally with code like this:

(&parent as *const _ as *const u8).add(offset_of!($ParentType, $field)) as *const $FieldType

This should create a pointer with the proper provenance which can be used with container_of.

@RalfJung
Copy link
Collaborator

RalfJung commented Aug 5, 2020

offset_of! is implemented using raw_field! so I am somewhat confused.^^

If you do core::ptr::raw_const!(struct.field), then doing container_of! on the resulting raw pointer should work. That is what I meant by only using raw pointers everywhere.

@RalfJung
Copy link
Collaborator

RalfJung commented Aug 5, 2020

Or maybe you were not talking about

macro_rules! raw_field {

but some other raw_field?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants