From 4fe3354c8f78cc140c71ea9897cd663ac33dde11 Mon Sep 17 00:00:00 2001 From: Jack May Date: Tue, 26 Oct 2021 13:07:40 -0700 Subject: [PATCH] Instruction sysvar fixes, additions (#20958) --- .../rust/instruction_introspection/src/lib.rs | 6 +- programs/bpf/rust/sysvar/src/lib.rs | 2 +- programs/bpf/tests/programs.rs | 12 +- sdk/program/src/sysvar/instructions.rs | 233 ++++++++++++++++-- 4 files changed, 228 insertions(+), 25 deletions(-) diff --git a/programs/bpf/rust/instruction_introspection/src/lib.rs b/programs/bpf/rust/instruction_introspection/src/lib.rs index 3798379a6..f39c811f4 100644 --- a/programs/bpf/rust/instruction_introspection/src/lib.rs +++ b/programs/bpf/rust/instruction_introspection/src/lib.rs @@ -36,11 +36,9 @@ fn process_instruction( let instruction = instructions::load_instruction_at_checked( secp_instruction_index as usize, instruction_accounts, - ) - .map_err(|_| ProgramError::InvalidAccountData)?; + )?; - let current_instruction = - instructions::load_current_index(&instruction_accounts.try_borrow_data()?); + let current_instruction = instructions::load_current_index_checked(instruction_accounts)?; let my_index = instruction_data[1] as u16; assert_eq!(current_instruction, my_index); diff --git a/programs/bpf/rust/sysvar/src/lib.rs b/programs/bpf/rust/sysvar/src/lib.rs index 6881edc82..135a3a167 100644 --- a/programs/bpf/rust/sysvar/src/lib.rs +++ b/programs/bpf/rust/sysvar/src/lib.rs @@ -48,7 +48,7 @@ pub fn process_instruction( msg!("Instructions identifier:"); sysvar::instructions::id().log(); assert_eq!(*accounts[4].owner, sysvar::id()); - let index = instructions::load_current_index(&accounts[4].try_borrow_data()?); + let index = instructions::load_current_index_checked(&accounts[4])?; let instruction = instructions::load_instruction_at_checked(index as usize, &accounts[4])?; assert_eq!(0, index); assert_eq!( diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 5c295a91a..55e4fc92b 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1388,17 +1388,17 @@ fn assert_instruction_count() { programs.extend_from_slice(&[ ("solana_bpf_rust_128bit", 584), ("solana_bpf_rust_alloc", 7388), - ("solana_bpf_rust_custom_heap", 512), + ("solana_bpf_rust_custom_heap", 535), ("solana_bpf_rust_dep_crate", 47), - ("solana_bpf_rust_external_spend", 483), + ("solana_bpf_rust_external_spend", 506), ("solana_bpf_rust_iter", 824), ("solana_bpf_rust_many_args", 941), - ("solana_bpf_rust_mem", 3083), + ("solana_bpf_rust_mem", 3085), ("solana_bpf_rust_membuiltins", 3976), - ("solana_bpf_rust_noop", 457), + ("solana_bpf_rust_noop", 480), ("solana_bpf_rust_param_passing", 146), - ("solana_bpf_rust_rand", 464), - ("solana_bpf_rust_sanity", 1714), + ("solana_bpf_rust_rand", 487), + ("solana_bpf_rust_sanity", 1716), ("solana_bpf_rust_secp256k1_recover", 25216), ("solana_bpf_rust_sha", 30704), ]); diff --git a/sdk/program/src/sysvar/instructions.rs b/sdk/program/src/sysvar/instructions.rs index ff4ab0cce..df43c0c8b 100644 --- a/sdk/program/src/sysvar/instructions.rs +++ b/sdk/program/src/sysvar/instructions.rs @@ -24,7 +24,12 @@ pub fn construct_instructions_data( data } -/// Load the current instruction's index from the Instructions Sysvar data +/// Load the current `Instruction`'s index in the currently executing +/// `Transaction` from the Instructions Sysvar data +#[deprecated( + since = "1.8.0", + note = "Unsafe because the sysvar accounts address is not checked, please use `load_current_index_checked` instead" +)] pub fn load_current_index(data: &[u8]) -> u16 { let mut instr_fixed_data = [0u8; 2]; let len = data.len(); @@ -32,13 +37,30 @@ pub fn load_current_index(data: &[u8]) -> u16 { u16::from_le_bytes(instr_fixed_data) } -/// Store the current instruction's index in the Instructions Sysvar data +/// Load the current `Instruction`'s index in the currently executing +/// `Transaction` +pub fn load_current_index_checked( + instruction_sysvar_account_info: &AccountInfo, +) -> Result { + if !check_id(instruction_sysvar_account_info.key) { + return Err(ProgramError::UnsupportedSysvar); + } + + let instruction_sysvar = instruction_sysvar_account_info.try_borrow_data()?; + let mut instr_fixed_data = [0u8; 2]; + let len = instruction_sysvar.len(); + instr_fixed_data.copy_from_slice(&instruction_sysvar[len - 2..len]); + Ok(u16::from_le_bytes(instr_fixed_data)) +} + +/// Store the current `Instruction`'s index in the Instructions Sysvar data pub fn store_current_index(data: &mut [u8], instruction_index: u16) { let last_index = data.len() - 2; data[last_index..last_index + 2].copy_from_slice(&instruction_index.to_le_bytes()); } -/// Load an instruction at the specified index +/// Load an `Instruction` in the currently executing `Transaction` at the +/// specified index #[deprecated( since = "1.8.0", note = "Unsafe because the sysvar accounts address is not checked, please use `load_instruction_at_checked` instead" @@ -47,7 +69,8 @@ pub fn load_instruction_at(index: usize, data: &[u8]) -> Result Result { + if !check_id(instruction_sysvar_account_info.key) { + return Err(ProgramError::UnsupportedSysvar); + } + + let instruction_sysvar = instruction_sysvar_account_info.data.borrow(); + #[allow(deprecated)] + let current_index = load_current_index(&instruction_sysvar) as i64; + let index = current_index.saturating_add(index_relative_to_current); + if index < 0 { + return Err(ProgramError::InvalidArgument); + } + #[allow(deprecated)] + load_instruction_at( + current_index.saturating_add(index_relative_to_current) as usize, + &instruction_sysvar, + ) + .map_err(|err| match err { + SanitizeError::IndexOutOfBounds => ProgramError::InvalidArgument, + _ => ProgramError::InvalidInstructionData, + }) +} + #[cfg(test)] mod tests { use super::*; @@ -75,24 +126,26 @@ mod tests { fn test_load_store_instruction() { let mut data = [4u8; 10]; store_current_index(&mut data, 3); - assert_eq!(load_current_index(&data), 3); + #[allow(deprecated)] + let index = load_current_index(&data); + assert_eq!(index, 3); assert_eq!([4u8; 8], data[0..8]); } #[test] fn test_load_instruction_at_checked() { + let instruction0 = Instruction::new_with_bincode( + Pubkey::new_unique(), + &0, + vec![AccountMeta::new(Pubkey::new_unique(), false)], + ); let instruction1 = Instruction::new_with_bincode( Pubkey::new_unique(), &0, vec![AccountMeta::new(Pubkey::new_unique(), false)], ); - let instruction2 = Instruction::new_with_bincode( - Pubkey::new_unique(), - &0, - vec![AccountMeta::new(Pubkey::new_unique(), false)], - ); let sanitized_message = crate::message::SanitizedMessage::try_from(Message::new( - &[instruction1.clone(), instruction2.clone()], + &[instruction0.clone(), instruction1.clone()], Some(&Pubkey::new_unique()), )) .unwrap(); @@ -101,7 +154,7 @@ mod tests { let mut lamports = 0; let mut data = construct_instructions_data(&sanitized_message, true); let owner = crate::sysvar::id(); - let account_info = AccountInfo::new( + let mut account_info = AccountInfo::new( &key, false, false, @@ -113,16 +166,168 @@ mod tests { ); assert_eq!( - instruction1, + instruction0, load_instruction_at_checked(0, &account_info).unwrap() ); assert_eq!( - instruction2, + instruction1, load_instruction_at_checked(1, &account_info).unwrap() ); assert_eq!( Err(ProgramError::InvalidArgument), load_instruction_at_checked(2, &account_info) ); + + let key = Pubkey::new_unique(); + account_info.key = &key; + assert_eq!( + Err(ProgramError::UnsupportedSysvar), + load_instruction_at_checked(2, &account_info) + ); + } + + #[test] + fn test_load_current_index_checked() { + let instruction0 = Instruction::new_with_bincode( + Pubkey::new_unique(), + &0, + vec![AccountMeta::new(Pubkey::new_unique(), false)], + ); + let instruction1 = Instruction::new_with_bincode( + Pubkey::new_unique(), + &0, + vec![AccountMeta::new(Pubkey::new_unique(), false)], + ); + let sanitized_message = crate::message::SanitizedMessage::try_from(Message::new( + &[instruction0, instruction1], + Some(&Pubkey::new_unique()), + )) + .unwrap(); + + let key = id(); + let mut lamports = 0; + let mut data = construct_instructions_data(&sanitized_message, true); + store_current_index(&mut data, 1); + let owner = crate::sysvar::id(); + let mut account_info = AccountInfo::new( + &key, + false, + false, + &mut lamports, + &mut data, + &owner, + false, + 0, + ); + + assert_eq!(1, load_current_index_checked(&account_info).unwrap()); + { + let mut data = account_info.try_borrow_mut_data().unwrap(); + store_current_index(&mut data, 0); + } + assert_eq!(0, load_current_index_checked(&account_info).unwrap()); + + let key = Pubkey::new_unique(); + account_info.key = &key; + assert_eq!( + Err(ProgramError::UnsupportedSysvar), + load_current_index_checked(&account_info) + ); + } + + #[test] + fn test_get_instruction_relative() { + let instruction0 = Instruction::new_with_bincode( + Pubkey::new_unique(), + &0, + vec![AccountMeta::new(Pubkey::new_unique(), false)], + ); + let instruction1 = Instruction::new_with_bincode( + Pubkey::new_unique(), + &0, + vec![AccountMeta::new(Pubkey::new_unique(), false)], + ); + let instruction2 = Instruction::new_with_bincode( + Pubkey::new_unique(), + &0, + vec![AccountMeta::new(Pubkey::new_unique(), false)], + ); + let sanitized_message = crate::message::SanitizedMessage::try_from(Message::new( + &[ + instruction0.clone(), + instruction1.clone(), + instruction2.clone(), + ], + Some(&Pubkey::new_unique()), + )) + .unwrap(); + + let key = id(); + let mut lamports = 0; + let mut data = construct_instructions_data(&sanitized_message, true); + store_current_index(&mut data, 1); + let owner = crate::sysvar::id(); + let mut account_info = AccountInfo::new( + &key, + false, + false, + &mut lamports, + &mut data, + &owner, + false, + 0, + ); + + assert_eq!( + Err(ProgramError::InvalidArgument), + get_instruction_relative(-2, &account_info) + ); + assert_eq!( + instruction0, + get_instruction_relative(-1, &account_info).unwrap() + ); + assert_eq!( + instruction1, + get_instruction_relative(0, &account_info).unwrap() + ); + assert_eq!( + instruction2, + get_instruction_relative(1, &account_info).unwrap() + ); + assert_eq!( + Err(ProgramError::InvalidArgument), + get_instruction_relative(2, &account_info) + ); + { + let mut data = account_info.try_borrow_mut_data().unwrap(); + store_current_index(&mut data, 0); + } + assert_eq!( + Err(ProgramError::InvalidArgument), + get_instruction_relative(-1, &account_info) + ); + assert_eq!( + instruction0, + get_instruction_relative(0, &account_info).unwrap() + ); + assert_eq!( + instruction1, + get_instruction_relative(1, &account_info).unwrap() + ); + assert_eq!( + instruction2, + get_instruction_relative(2, &account_info).unwrap() + ); + assert_eq!( + Err(ProgramError::InvalidArgument), + get_instruction_relative(3, &account_info) + ); + + let key = Pubkey::new_unique(); + account_info.key = &key; + assert_eq!( + Err(ProgramError::UnsupportedSysvar), + get_instruction_relative(0, &account_info) + ); } }