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

__BindgenUnionField and __IncompleteArrayField are UB according to Stacked Borrows model. #1892

Closed
Dirbaio opened this issue Sep 8, 2020 · 9 comments

Comments

@Dirbaio
Copy link

Dirbaio commented Sep 8, 2020

Generated helpers __BindgenUnionField and __IncompleteArrayField are impossible to use without invoking UB according to the Stacked Borrows model.

The "short" explanation for why is that raw pointers created from a '&T' can only be used to access memory inside the T. The pointers "remember" that they come from a &T. I don't think I fully grasp Stacked Borrows to give more detail though 😓

These two structs are zero-sized, so methods taking &self are forbidden to read memory "outside" self, even through pointer casts.

Quick playground demo. To see the UB, run MIRI from the 'Tools' menu.

There's a possible workaround that works by calculating the offset of the member, and using it to construct a new "sanitized" pointer from the "original" pointer, that's not "tainted" by coming from a reference. See slice_from_flexarray in the playground above.

@Dirbaio
Copy link
Author

Dirbaio commented Sep 8, 2020

See here for more context: rust-lang/unsafe-code-guidelines#134 (comment)

@emilio
Copy link
Contributor

emilio commented Sep 9, 2020

Hmm... sure, though I don't get how slice_from_flexarray is also not UB. It seems you're just fooling miri. For example this version of as_slice doesn't make miri complain:

    #[inline]
    pub unsafe fn as_slice(&self, len: usize) -> &[T] {
        let ptr = self as *const _ as usize;
        ::std::slice::from_raw_parts(ptr as *const u8 as *const T, len)
    }

I don't think that's fundamentally different though.

@emilio
Copy link
Contributor

emilio commented Sep 9, 2020

cc @RalfJung in case the above is a miri bug or what not...

But TLDR I think this should work as is to be fair. If there is a safer way to do it I'm all ears, or if the version of as_slice above is not UB somehow, I'd also love to know why. But other than that this doesn't seem actionable on bindgen right now.

@emilio emilio closed this as completed Sep 9, 2020
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

rust-lang/unsafe-code-guidelines#134 is indeed the right reference. This is not a Miri bug, it is working as designed, but it might be a weakness of the design. Whether it is possible to resolve that without giving up many useful optimizations is still an open question.

The keyword "flexarray" reminds me of a few discussions I had with @retep998 who I think developed a sound API for C-style flexible arrays. I think their approach was very similar to slice_from_flexarray. The key is that you want to make sure you have a pointer with the right provenance, ensuring it can access the entire array, not just the zero-sized pseudo-element at the beginning.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

Also, could this code entirely avoid the use of references once rust-lang/rust#73394 is stable? That is probably a much more sane solution that the "sensitization" trick.

@retep998
Copy link
Member

For reference this is the API I use for working with such variable sized structs: https://github.com/retep998/wio-rs/blob/master/src/vsb.rs

@Easyoakland
Copy link

Easyoakland commented May 15, 2024

@emilio

Hmm... sure, though I don't get how slice_from_flexarray is also not UB. It seems you're just fooling miri. For example this version of as_slice doesn't make miri complain:

    #[inline]
    pub unsafe fn as_slice(&self, len: usize) -> &[T] {
        let ptr = self as *const _ as usize;
        ::std::slice::from_raw_parts(ptr as *const u8 as *const T, len)
    }

I don't think that's fundamentally different though.

Maybe something changed since you wrote this, but that is not accepted by miri

trying to retag from for SharedReadOnly permission

while the original sanitized pointer solution is accepted by miri.

@RalfJung
Copy link
Member

Note that when there is the "warning: integer-to-pointer cast", even if Miri does not complain, that doesn't mean much. So if your "sanitized pointer solution" involves int2ptr casts, it can't really be tested in Miri.

Also this is a 4-year-old closed PR, not a suitable place for discussion. Not sure what you're trying to achieve but I suggest opening a new issue if there's something that should/could be improved.

@emilio
Copy link
Contributor

emilio commented May 16, 2024

Yes, please. FWIW this is effectively rust-lang/unsafe-code-guidelines#256 which is still open.

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

No branches or pull requests

5 participants