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

Use MaybeUninit more properly in streamer::batch_send() #3325

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

steviez
Copy link

@steviez steviez commented Oct 27, 2024

Problem

batch_send() has undefined behavior. Some uninitialized data is marked as initialized with assume_init() even though it hasn't actually been initialized:

#[allow(clippy::uninit_assumed_init)]
let iovec = std::mem::MaybeUninit::<iovec>::uninit();
let mut iovs = vec![unsafe { iovec.assume_init() }; size];

This is problematic since the iovec struct has a pointer:

#[repr(C)]
pub struct iovec {
    pub iov_base: *mut c_void,
    pub iov_len: size_t,
}

With rust 1.82, this made the validator start crashing:

kernel: traps: solRspndrGossip[1514252] trap invalid opcode ip:561a11d83bb7 sp:7eb14c3a4830 error:0
sol.service: Main process exited, code=killed, status=4/ILL

Summary of Changes

  • Use MaybeUninit interface to initialize iovec's
  • Use MaybeUninit interface to initialize mmsghdr's
  • Write every field in mmsghdr so that we no longer need to request zeroed data
  • Use MaybeUninit interface to initialize sockaddr_storage's

@steviez
Copy link
Author

steviez commented Oct 27, 2024

Two things as I review this real quick:

  • Should probably refactor addrs and hdrs to use MaybeUninit API as well
  • recv_mmsg() has similar issues as batch_send(); I prefer smaller PR's so am initially inclined to do that separately but TBD

@steviez steviez changed the title Avoid assume_init() on uninitialized struct in streamer::batch_send() Use MaybeUninit more properly in streamer::batch_send() Oct 28, 2024
alessandrod
alessandrod previously approved these changes Oct 28, 2024
streamer/src/sendmmsg.rs Outdated Show resolved Hide resolved
streamer/src/sendmmsg.rs Outdated Show resolved Hide resolved
@steviez steviez merged commit 76dd7b2 into anza-xyz:master Oct 29, 2024
40 checks passed
@steviez steviez deleted the streamer_send_ub branch October 29, 2024 01:18
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
An array of iovec was previously created as a MaybeUninit, and then
immediately marked as initialized with assume_init(). This was likely
a workaround was iovec not having a Default implementation, but this
also created undefined behavior since the iovec type has pointers. With
the bump to rust 1.82, this was causing the validator to crash

This change uses the MaybeUninit interface to more properly create the
libc types as MaybeUninit, initialize them, and then transmute them
to their regular, initialized counterpart types
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