Remove unsound offset_of macro #1485
Conversation
Thomasdezeeuw
force-pushed
the
issue_1483
branch
from
39ecb88 to
a8e122f
Compare
And replace it with constants that define the offsets to the fields. It's not a pretty solution, but it's one without UB.
Thomasdezeeuw
force-pushed
the
issue_1483
branch
from
a8e122f to
5cd564c
Compare
Moving the Overlapped fields to the start to make it easier to determine the offsets and hopefully incur less breakage once external fields change size. Note that the Overlapped fields internally uses miow::Overlapped, which in turn is a OVERLAPPED struct as found in the winapi crate and has a stable layout (as defined by the Windows API).
src/sys/windows/named_pipe.rs
Outdated
| /// fields. | ||
| const CONNECT_OFFSET: usize = 0; | ||
| const READ_OFFSET: usize = CONNECT_OFFSET + size_of::<Overlapped>(); // `connect` fields. | ||
| const WRITE_OFFSET: usize = READ_OFFSET + size_of::<Overlapped>(); // `read` fields. |
There was a problem hiding this comment.
This seems rather fragile...
If I understand correctly, we actually do have an instance of the relevant type available at the place where the offset needs to be computed. So why can't the code simply compute the offsets based on that instance? offset_of! is only hard when you don't have a value of the type you want to compute offsets in.
There was a problem hiding this comment.
I agree it's fragile.
But we don't have an instance available in read_done when we need it. We have a pointer to one of the fields of the instance. How do you suggest we get the base pointer (to Inner) then?
There was a problem hiding this comment.
Ah, I thought you'd have an instance anywhere, sorry... I don't really understand the code.^^
Btw, this looks like container_of!-style pointer arithmetic, which does not work well with the Rust aliasing model... rust-lang/unsafe-code-guidelines#243.
But ignoring aliasing issues, if you do not want to depend on addr_of!, you could either decide to not care about code being technically UB (you could still avoid the NULL ptr by using NonNull::dangling as the base) -- or I guess hard-code the offsets as you do...
There was a problem hiding this comment.
I don't really know of a way to solve this. I'm following rust-lang/unsafe-code-guidelines#243 now so if someone suggests a solution that is not UB we can always update this code.
Changes the Inner::ptr_from_* methods to use ptr::wrapping_sub rather then casting to usize.
|
CI failures: cross compile for Solaris fails; got a pr for it #1487, Minimal Windows versions fail with Cargo not being installed, but other Windows versions pass just fine.. I don't know what is going on there. |
rust-lang/rust#82216 removed the x86_64-sun-solaris target from rustup, changing it to use x86_64-pc-solaris instead. Related issues: * rust-lang/rust#85098
And replace it with constants that define the offsets to the fields.
It's not a pretty solution, but it's one without UB.
Closes #1483.