Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
alessandrod committed Aug 1, 2024
1 parent 4f53143 commit 69baa4a
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 18 deletions.
15 changes: 8 additions & 7 deletions sdk/program/src/serialize_utils/cursor.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use {
crate::{instruction::InstructionError, pubkey::Pubkey},
crate::{
instruction::InstructionError,
pubkey::{Pubkey, PUBKEY_BYTES},
},
std::{
io::{BufRead as _, Cursor, Read},
mem, ptr,
ptr,
},
};

Expand Down Expand Up @@ -57,18 +60,16 @@ pub(crate) fn read_pubkey_into(
cursor: &mut Cursor<&[u8]>,
pubkey: *mut Pubkey,
) -> Result<(), InstructionError> {
const PUBKEY_SIZE: usize = mem::size_of::<Pubkey>();

match cursor.fill_buf() {
Ok(buf) if buf.len() >= PUBKEY_SIZE => {
Ok(buf) if buf.len() >= PUBKEY_BYTES => {
// Safety: `buf` is guaranteed to be at least `PUBKEY_SIZE` bytes
// long. Pubkey a #[repr(transparent)] wrapper around a byte array,
// so this is a byte to byte copy and it's safe.
unsafe {
ptr::copy_nonoverlapping(buf.as_ptr(), pubkey as *mut u8, PUBKEY_SIZE);
ptr::copy_nonoverlapping(buf.as_ptr(), pubkey as *mut u8, PUBKEY_BYTES);
}

cursor.consume(PUBKEY_SIZE);
cursor.consume(PUBKEY_BYTES);
}
_ => return Err(InstructionError::InvalidAccountData),
}
Expand Down
34 changes: 24 additions & 10 deletions sdk/program/src/vote/state/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Vote state
use std::{mem, ptr};

#[cfg(not(target_os = "solana"))]
use bincode::deserialize;
#[cfg(test)]
Expand Down Expand Up @@ -492,7 +494,7 @@ impl VoteState {
) -> Result<(), InstructionError> {
// Rebind vote_state to *mut VoteState so that the &mut binding isn't
// accessible anymore, preventing accidental use after this point.
let vote_state = vote_state as *mut VoteState;
let vote_state = ptr::from_mut(vote_state);

// Safety: vote_state is valid to_drop (see drop_in_place() docs). After
// dropping, the pointer is treated as uninitialized and only accessed
Expand All @@ -501,24 +503,36 @@ impl VoteState {
std::ptr::drop_in_place(vote_state);
}

match VoteState::deserialize_into_ptr(input, vote_state) {
Ok(()) => Ok(()),
Err(err) => {
// This is to reset vote_state to VoteState::default() if deserialize fails or panics.
struct DropGuard {
vote_state: *mut VoteState,
}

impl Drop for DropGuard {
fn drop(&mut self) {
// Safety:
//
// Deserialize failed so at this point vote_state is uninitialized. We must write a
// new _valid_ value into it or after returning from this function the caller is
// left with an uninitialized `&mut VoteState`, which is UB (references must always
// be valid).
// Deserialize failed or panicked so at this point vote_state is uninitialized. We
// must write a new _valid_ value into it or after returning (or unwinding) from
// this function the caller is left with an uninitialized `&mut VoteState`, which is
// UB (references must always be valid).
//
// This is always safe and doesn't leak memory because deserialize_into_ptr() writes
// into the fields that heap alloc only when it returns Ok().
unsafe {
vote_state.write(VoteState::default());
self.vote_state.write(VoteState::default());
}
Err(err)
}
}

let guard = DropGuard { vote_state };

let res = VoteState::deserialize_into_ptr(input, vote_state);
if res.is_ok() {
mem::forget(guard);
}

res
}

/// Deserializes the input `VoteStateVersions` buffer directly into the provided
Expand Down
3 changes: 2 additions & 1 deletion sdk/program/src/vote/state/vote_state_deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ fn read_prior_voters_into<T: AsRef<[u8]>>(

fn read_epoch_credits<T: AsRef<[u8]>>(
cursor: &mut Cursor<T>,
) -> Result<Vec<(u64, u64, u64)>, InstructionError> {
) -> Result<Vec<(Epoch, u64, u64)>, InstructionError> {
let epoch_credit_count = read_u64(cursor)? as usize;
let mut epoch_credits = Vec::with_capacity(epoch_credit_count.min(MAX_EPOCH_CREDITS_HISTORY));

Expand All @@ -142,6 +142,7 @@ fn read_last_timestamp_into<T: AsRef<[u8]>>(

let last_timestamp = BlockTimestamp { slot, timestamp };

// Safety: if vote_state is non-null, last_timestamp is guaranteed to be valid too
unsafe {
addr_of_mut!((*vote_state).last_timestamp).write(last_timestamp);
}
Expand Down

0 comments on commit 69baa4a

Please sign in to comment.