Skip to content

Ensure macOS pointer addresses are aligned to eight bytes #313

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Ensure macOS pointer addresses are aligned to eight bytes #313

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 72 additions & 26 deletions src/platform/macos/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,7 @@ impl OsIpcSender {
unsafe {
let size = Message::size_of(&data, ports.len(), shared_memory_regions.len());
let message = libc::malloc(size as size_t) as *mut Message;
println!("{:p}", message);
(*message).header.msgh_bits = (MACH_MSG_TYPE_COPY_SEND as u32) |
MACH_MSGH_BITS_COMPLEX;
(*message).header.msgh_size = size as u32;
Expand All @@ -500,34 +501,38 @@ impl OsIpcSender {
(*message).body.msgh_descriptor_count =
(ports.len() + shared_memory_regions.len()) as u32;

let mut port_descriptor_dest = message.offset(1) as *mut mach_msg_port_descriptor_t;
let mut port_descriptor_dest = (message as *mut PaddedMessage).offset(1) as *mut PaddedPortDescriptor;
Copy link
Member

@mukilan mukilan Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdm , I was able to fix the test failures with these changes.

I'm not 100% sure about this, but this is my theory of what is causing the tests to fail: Because padding is added to the wrapped mach_msg_port_descriptor_t & mach_ool_descriptor_t structs, the entries in the array of descriptors following Message struct will have the wrong layout from the perspective of kernel. Since the kernel needs to interpret the values in these fields for validating the transfer of ports, it could end up reading random invalid data as the port names.

I couldn't find any documentation explaining the alignment requirements for OOL descriptor entries so I assumed they are byte-aligned (so address is now [u8; 8]). Relaxing the alignment requirement allows the debug assertions inserted by rust on the reads/writes to succeed. The tests platform::test::shared_memory and platform::test:shared_memory_clone used to pass with Rust 1.67 and now continue to pass with my changes, so I think the Kernel doesn't have the 8-byte alignment requirement (for the descriptors) either.

I can raise a PR if the changes seem fine to you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! I'm going to need to sit and think about this solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this I think the kernel only expects 4-byte alignment. It also seems rust-bindgen didn't translate pragma pack directives until 2018 and the mach_sys.rs was added in 2015.

Should we just regenerate the mach_sys.rs using newer rust-bindgen? If so, I don't know which header file is the official one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that makes a lot of sense! I'd say give rust-bindgen a try on the header that you linked and see what the output looks like?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I guess https://github.com/apple-oss-distributions/xnu/blob/main/osfmk/mach/message.h is the official distribution now.

println!("{:p}", port_descriptor_dest);
for outgoing_port in &ports {
(*port_descriptor_dest).name = outgoing_port.port();
(*port_descriptor_dest).pad1 = 0;
(*port_descriptor_dest).descriptor.name = outgoing_port.port();
(*port_descriptor_dest).descriptor.pad1 = 0;

(*port_descriptor_dest).disposition = match *outgoing_port {
(*port_descriptor_dest).descriptor.disposition = match *outgoing_port {
OsIpcChannel::Sender(_) => MACH_MSG_TYPE_MOVE_SEND,
OsIpcChannel::Receiver(_) => MACH_MSG_TYPE_MOVE_RECEIVE,
};

(*port_descriptor_dest).type_ = MACH_MSG_PORT_DESCRIPTOR;
(*port_descriptor_dest).descriptor.type_ = MACH_MSG_PORT_DESCRIPTOR;
port_descriptor_dest = port_descriptor_dest.offset(1);
}

let mut shared_memory_descriptor_dest =
port_descriptor_dest as *mut mach_msg_ool_descriptor_t;
port_descriptor_dest as *mut PaddedOOLDescriptor;
println!("{:p}", shared_memory_descriptor_dest);
for shared_memory_region in &shared_memory_regions {
(*shared_memory_descriptor_dest).address =
(*shared_memory_descriptor_dest).descriptor.address =
shared_memory_region.as_ptr() as *const c_void as *mut c_void;
(*shared_memory_descriptor_dest).size = shared_memory_region.len() as u32;
(*shared_memory_descriptor_dest).deallocate = 1;
(*shared_memory_descriptor_dest).copy = MACH_MSG_VIRTUAL_COPY as u8;
(*shared_memory_descriptor_dest).type_ = MACH_MSG_OOL_DESCRIPTOR;
(*shared_memory_descriptor_dest).descriptor.size = shared_memory_region.len() as u32;
(*shared_memory_descriptor_dest).descriptor.deallocate = 1;
(*shared_memory_descriptor_dest).descriptor.copy = MACH_MSG_VIRTUAL_COPY as u8;
(*shared_memory_descriptor_dest).descriptor.type_ = MACH_MSG_OOL_DESCRIPTOR;
(*shared_memory_descriptor_dest).padding = [0; mach_msg_ool_descriptor_t::padding()];
shared_memory_descriptor_dest = shared_memory_descriptor_dest.offset(1);
}

let is_inline_dest = shared_memory_descriptor_dest as *mut bool;
*is_inline_dest = data.is_inline();
let is_inline_dest = shared_memory_descriptor_dest as *mut usize;
println!("{:p}", is_inline_dest);
*is_inline_dest = data.is_inline() as usize;

if data.is_inline() {
// Zero out the last word for paranoia's sake.
Expand All @@ -536,6 +541,7 @@ impl OsIpcSender {
let data = data.inline_data();
let data_size = data.len();
let data_size_dest = is_inline_dest.offset(1) as *mut usize;
println!("{:p}", data_size_dest);
*data_size_dest = data_size;

let data_dest = data_size_dest.offset(1) as *mut u8;
Expand Down Expand Up @@ -738,35 +744,36 @@ fn select(port: mach_port_t, blocking_mode: BlockingMode)
os_result => return Err(MachError::from(os_result)),
}

let padded_message = message as *mut PaddedMessage;
let local_port = (*message).header.msgh_local_port;
if (*message).header.msgh_id == MACH_NOTIFY_NO_SENDERS {
return Ok(OsIpcSelectionResult::ChannelClosed(local_port as u64))
}

let (mut ports, mut shared_memory_regions) = (Vec::new(), Vec::new());
let mut port_descriptor = message.offset(1) as *mut mach_msg_port_descriptor_t;
let mut port_descriptor = (message as *mut PaddedMessage).offset(1) as *mut PaddedPortDescriptor;
let mut descriptors_remaining = (*message).body.msgh_descriptor_count;
while descriptors_remaining > 0 {
if (*port_descriptor).type_ != MACH_MSG_PORT_DESCRIPTOR {
if (*port_descriptor).descriptor.type_ != MACH_MSG_PORT_DESCRIPTOR {
break
}
ports.push(OsOpaqueIpcChannel::from_name((*port_descriptor).name));
ports.push(OsOpaqueIpcChannel::from_name((*port_descriptor).descriptor.name));
port_descriptor = port_descriptor.offset(1);
descriptors_remaining -= 1;
}

let mut shared_memory_descriptor = port_descriptor as *mut mach_msg_ool_descriptor_t;
let mut shared_memory_descriptor = port_descriptor as *mut PaddedOOLDescriptor;
while descriptors_remaining > 0 {
debug_assert!((*shared_memory_descriptor).type_ == MACH_MSG_OOL_DESCRIPTOR);
debug_assert!((*shared_memory_descriptor).descriptor.type_ == MACH_MSG_OOL_DESCRIPTOR);
shared_memory_regions.push(OsIpcSharedMemory::from_raw_parts(
(*shared_memory_descriptor).address as *mut u8,
(*shared_memory_descriptor).size as usize));
(*shared_memory_descriptor).descriptor.address as *mut u8,
(*shared_memory_descriptor).descriptor.size as usize));
shared_memory_descriptor = shared_memory_descriptor.offset(1);
descriptors_remaining -= 1;
}

let has_inline_data_ptr = shared_memory_descriptor as *mut bool;
let has_inline_data = *has_inline_data_ptr;
let has_inline_data_ptr = shared_memory_descriptor as *mut usize;
let has_inline_data = *has_inline_data_ptr != 0;
let payload = if has_inline_data {
let payload_size_ptr = has_inline_data_ptr.offset(1) as *mut usize;
let payload_size = *payload_size_ptr;
Expand Down Expand Up @@ -942,12 +949,51 @@ struct Message {
body: mach_msg_body_t,
}

#[repr(C)]
struct PaddedMessage {
message: Message,
padding: [u8; Message::padding()],
}

#[repr(C)]
struct PaddedPortDescriptor {
descriptor: mach_msg_port_descriptor_t,
padding: [u8; mach_msg_port_descriptor_t::padding()],
}

#[repr(C)]
struct PaddedOOLDescriptor {
descriptor: mach_msg_ool_descriptor_t,
padding: [u8; mach_msg_ool_descriptor_t::padding()],
}

impl mach_msg_port_descriptor_t {
const fn padding() -> usize {
//mem::size_of::<Self>() % 8
mem::align_of::<Self>()
}
}

impl mach_msg_ool_descriptor_t {
const fn padding() -> usize {
//mem::size_of::<Self>() % 8
mem::align_of::<Self>()

}
}

impl Message {
const fn padding() -> usize {
//mem::size_of::<Self>() % 8
mem::align_of::<Self>()
}

fn size_of(data: &SendData, port_length: usize, shared_memory_length: usize) -> usize {
let mut size = mem::size_of::<Message>() +
mem::size_of::<mach_msg_port_descriptor_t>() * port_length +
mem::size_of::<mach_msg_ool_descriptor_t>() * shared_memory_length +
mem::size_of::<bool>();
println!("padding: {}, {}, {}", Self::padding(), mach_msg_port_descriptor_t::padding(), mach_msg_ool_descriptor_t::padding());
let mut size = mem::size_of::<PaddedMessage>() +
mem::size_of::<PaddedPortDescriptor>() * port_length +
mem::size_of::<PaddedOOLDescriptor>() * shared_memory_length +
mem::size_of::<usize>();

if data.is_inline() {
size += mem::size_of::<usize>() + data.inline_data().len();
Expand Down