From 1df88837c8d11c40bcdf2800ecc4606493bbfd67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 7 Dec 2021 23:00:04 +0100 Subject: [PATCH] - Implicitly fixes invoke_context.return_data not being reset between instructions in process_message. (#21671) - Lets InvokeContext::process_cross_program_instruction() handle the first invocation depth too. - Marks InvokeContext::verify(), InvokeContext::verify_and_update() and InvokeContext::process_executable_chain() private. - Renames InvokeContext::process_cross_program_instruction() to InvokeContext::process_instruction(). - Removes InvokeContext::new_mock_with_sysvars(). --- program-runtime/src/invoke_context.rs | 114 ++++++++++++----------- program-test/src/lib.rs | 3 +- programs/bpf_loader/src/serialization.rs | 2 +- programs/bpf_loader/src/syscalls.rs | 25 ++--- programs/stake/src/stake_instruction.rs | 4 +- rbpf-cli/src/main.rs | 2 +- runtime/src/message_processor.rs | 38 +++----- 7 files changed, 91 insertions(+), 97 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 5b775a97e5..bcff3fa614 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -192,20 +192,12 @@ impl<'a> InvokeContext<'a> { pub fn new_mock( accounts: &'a [(Pubkey, Rc>)], builtin_programs: &'a [BuiltinProgram], - ) -> Self { - Self::new_mock_with_sysvars(accounts, builtin_programs, &[]) - } - - pub fn new_mock_with_sysvars( - accounts: &'a [(Pubkey, Rc>)], - builtin_programs: &'a [BuiltinProgram], - sysvars: &'a [(Pubkey, Vec)], ) -> Self { Self::new( Rent::default(), accounts, builtin_programs, - sysvars, + &[], Some(LogCollector::new_ref()), ComputeBudget::default(), Rc::new(RefCell::new(Executors::default())), @@ -221,7 +213,7 @@ impl<'a> InvokeContext<'a> { message: &Message, instruction: &CompiledInstruction, program_indices: &[usize], - account_indices: Option<&[usize]>, + account_indices: &[usize], ) -> Result<(), InstructionError> { if self.invoke_stack.len() > self.compute_budget.max_invoke_depth { return Err(InstructionError::CallDepth); @@ -302,10 +294,10 @@ impl<'a> InvokeContext<'a> { }) .chain(instruction.accounts.iter().map(|index_in_instruction| { let index_in_instruction = *index_in_instruction as usize; - let account_index = if let Some(account_indices) = account_indices { - account_indices[index_in_instruction] - } else { + let account_index = if account_indices.is_empty() { index_in_instruction + } else { + account_indices[index_in_instruction] }; ( message.is_signer(index_in_instruction), @@ -334,7 +326,7 @@ impl<'a> InvokeContext<'a> { } /// Verify the results of an instruction - pub fn verify( + fn verify( &mut self, message: &Message, instruction: &CompiledInstruction, @@ -403,7 +395,7 @@ impl<'a> InvokeContext<'a> { } /// Verify and update PreAccount state based on program execution - pub fn verify_and_update( + fn verify_and_update( &mut self, instruction: &CompiledInstruction, account_indices: &[usize], @@ -505,8 +497,9 @@ impl<'a> InvokeContext<'a> { if let Some(instruction_recorder) = &self.instruction_recorder { instruction_recorder.record_instruction(instruction); } - self.process_cross_program_instruction( + self.process_instruction( &message, + &message.instructions[0], &program_indices, &account_indices, &caller_write_privileges, @@ -530,7 +523,7 @@ impl<'a> InvokeContext<'a> { Ok(()) } - /// Helper to prepare for process_cross_program_instruction() + /// Helper to prepare for process_instruction() pub fn create_message( &mut self, instruction: &Instruction, @@ -644,42 +637,50 @@ impl<'a> InvokeContext<'a> { } /// Process a cross-program instruction - pub fn process_cross_program_instruction( + pub fn process_instruction( &mut self, message: &Message, + instruction: &CompiledInstruction, program_indices: &[usize], account_indices: &[usize], caller_write_privileges: &[bool], ) -> Result<(), InstructionError> { - // This function is always called with a valid instruction, if that changes return an error - let instruction = message - .instructions - .get(0) - .ok_or(InstructionError::GenericError)?; + let is_lowest_invocation_level = self.invoke_stack.is_empty(); + if !is_lowest_invocation_level { + // Verify the calling program hasn't misbehaved + self.verify_and_update(instruction, account_indices, caller_write_privileges)?; + } - // Verify the calling program hasn't misbehaved - self.verify_and_update(instruction, account_indices, caller_write_privileges)?; + let result = self + .push(message, instruction, program_indices, account_indices) + .and_then(|_| { + self.return_data = (*instruction.program_id(&message.account_keys), Vec::new()); + self.process_executable_chain(&instruction.data)?; - self.return_data = (*self.get_caller()?, Vec::new()); - self.push(message, instruction, program_indices, Some(account_indices))?; - let result = self.process_instruction(&instruction.data).and_then(|_| { - // Verify the called program has not misbehaved - let demote_program_write_locks = self - .feature_set - .is_active(&demote_program_write_locks::id()); - let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i, demote_program_write_locks)) - .collect(); - self.verify_and_update(instruction, account_indices, &write_privileges) - }); + // Verify the called program has not misbehaved + if is_lowest_invocation_level { + self.verify(message, instruction, program_indices) + } else { + let demote_program_write_locks = self + .feature_set + .is_active(&demote_program_write_locks::id()); + let write_privileges: Vec = (0..message.account_keys.len()) + .map(|i| message.is_writable(i, demote_program_write_locks)) + .collect(); + self.verify_and_update(instruction, account_indices, &write_privileges) + } + }); - // Restore previous state + // Pop the invoke_stack to restore previous state self.pop(); result } /// Calls the instruction's program entrypoint method - pub fn process_instruction(&mut self, instruction_data: &[u8]) -> Result<(), InstructionError> { + fn process_executable_chain( + &mut self, + instruction_data: &[u8], + ) -> Result<(), InstructionError> { let keyed_accounts = self.get_keyed_accounts()?; let root_account = keyed_account_at_index(keyed_accounts, 0) .map_err(|_| InstructionError::UnsupportedProgramId)?; @@ -920,7 +921,7 @@ pub fn with_mock_invoke_context R>( &preparation.message, &preparation.message.instructions[0], &program_indices, - Some(&preparation.account_indices), + &preparation.account_indices, ) .unwrap(); callback(&mut invoke_context) @@ -939,13 +940,13 @@ pub fn mock_process_instruction_with_sysvars( let processor_account = AccountSharedData::new_ref(0, 0, &solana_sdk::native_loader::id()); program_indices.insert(0, preparation.accounts.len()); preparation.accounts.push((*loader_id, processor_account)); - let mut invoke_context = - InvokeContext::new_mock_with_sysvars(&preparation.accounts, &[], sysvars); + let mut invoke_context = InvokeContext::new_mock(&preparation.accounts, &[]); + invoke_context.sysvars = sysvars; invoke_context.push( &preparation.message, &preparation.message.instructions[0], &program_indices, - Some(&preparation.account_indices), + &preparation.account_indices, )?; process_instruction(1, instruction_data, &mut invoke_context) } @@ -1108,7 +1109,7 @@ mod tests { &message, &message.instructions[0], &[MAX_DEPTH + depth_reached], - None, + &[], ) { break; @@ -1187,7 +1188,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); assert!(invoke_context .verify(&message, &message.instructions[0], &[0]) @@ -1252,7 +1253,7 @@ mod tests { }]; let mut invoke_context = InvokeContext::new_mock(&accounts, builtin_programs); invoke_context - .push(&message, &caller_instruction, &program_indices[..1], None) + .push(&message, &caller_instruction, &program_indices[..1], &[]) .unwrap(); // not owned account modified by the caller (before the invoke) @@ -1267,8 +1268,9 @@ mod tests { .collect::>(); accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1; assert_eq!( - invoke_context.process_cross_program_instruction( + invoke_context.process_instruction( &message, + &message.instructions[0], &program_indices[1..], &account_indices, &caller_write_privileges, @@ -1280,8 +1282,9 @@ mod tests { // readonly account modified by the invoker accounts[2].1.borrow_mut().data_as_mut_slice()[0] = 1; assert_eq!( - invoke_context.process_cross_program_instruction( + invoke_context.process_instruction( &message, + &message.instructions[0], &program_indices[1..], &account_indices, &caller_write_privileges, @@ -1309,7 +1312,7 @@ mod tests { Instruction::new_with_bincode(callee_program_id, &case.0, metas.clone()); let message = Message::new(&[callee_instruction], None); invoke_context - .push(&message, &caller_instruction, &program_indices[..1], None) + .push(&message, &caller_instruction, &program_indices[..1], &[]) .unwrap(); let caller_write_privileges = message .account_keys @@ -1318,8 +1321,9 @@ mod tests { .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); assert_eq!( - invoke_context.process_cross_program_instruction( + invoke_context.process_instruction( &message, + &message.instructions[0], &program_indices[1..], &account_indices, &caller_write_privileges, @@ -1380,7 +1384,7 @@ mod tests { }]; let mut invoke_context = InvokeContext::new_mock(&accounts, builtin_programs); invoke_context - .push(&message, &caller_instruction, &program_indices, None) + .push(&message, &caller_instruction, &program_indices, &[]) .unwrap(); // not owned account modified by the invoker @@ -1423,7 +1427,7 @@ mod tests { Instruction::new_with_bincode(callee_program_id, &case.0, metas.clone()); let message = Message::new(&[callee_instruction.clone()], None); invoke_context - .push(&message, &caller_instruction, &program_indices, None) + .push(&message, &caller_instruction, &program_indices, &[]) .unwrap(); assert_eq!( invoke_context.native_invoke(callee_instruction, &[]), @@ -1470,7 +1474,7 @@ mod tests { invoke_context.feature_set = Arc::new(feature_set); invoke_context - .push(&noop_message, &noop_message.instructions[0], &[0], None) + .push(&noop_message, &noop_message.instructions[0], &[0], &[]) .unwrap(); assert_eq!( *invoke_context.get_compute_budget(), @@ -1479,7 +1483,7 @@ mod tests { invoke_context.pop(); invoke_context - .push(&neon_message, &neon_message.instructions[0], &[1], None) + .push(&neon_message, &neon_message.instructions[0], &[1], &[]) .unwrap(); let expected_compute_budget = ComputeBudget { max_units: 500_000, @@ -1493,7 +1497,7 @@ mod tests { invoke_context.pop(); invoke_context - .push(&noop_message, &noop_message.instructions[0], &[0], None) + .push(&noop_message, &noop_message.instructions[0], &[0], &[]) .unwrap(); assert_eq!( *invoke_context.get_compute_budget(), diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 77c4ffdefd..eebb76b7c0 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -319,8 +319,9 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { } invoke_context - .process_cross_program_instruction( + .process_instruction( &message, + &message.instructions[0], &program_indices, &account_indices, &caller_privileges, diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index cfde48561a..4ae530e21f 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -455,7 +455,7 @@ mod tests { &preparation.message, &preparation.message.instructions[0], &program_indices, - Some(&preparation.account_indices), + &preparation.account_indices, ) .unwrap(); diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 3c813c2f26..f1d97f858c 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -2390,8 +2390,9 @@ fn call<'a, 'b: 'a>( // Process instruction invoke_context - .process_cross_program_instruction( + .process_instruction( &message, + &message.instructions[0], &program_indices, &account_indices, &caller_write_privileges, @@ -2989,7 +2990,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall_panic = SyscallPanic { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3066,7 +3067,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall_sol_log = SyscallLog { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3170,7 +3171,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let cost = invoke_context.get_compute_budget().log_64_units; let mut syscall_sol_log_u64 = SyscallLogU64 { @@ -3212,7 +3213,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let cost = invoke_context.get_compute_budget().log_pubkey_units; let mut syscall_sol_pubkey = SyscallLogPubkey { @@ -3486,7 +3487,7 @@ mod tests { * 4, ); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall = SyscallSha256 { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3585,7 +3586,7 @@ mod tests { let sysvars = [(sysvar::clock::id(), data)]; invoke_context.sysvars = &sysvars; invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall = SyscallGetClockSysvar { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3630,7 +3631,7 @@ mod tests { let sysvars = [(sysvar::epoch_schedule::id(), data)]; invoke_context.sysvars = &sysvars; invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall = SyscallGetEpochScheduleSysvar { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3682,7 +3683,7 @@ mod tests { let sysvars = [(sysvar::fees::id(), data)]; invoke_context.sysvars = &sysvars; invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall = SyscallGetFeesSysvar { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3725,7 +3726,7 @@ mod tests { let sysvars = [(sysvar::rent::id(), data)]; invoke_context.sysvars = &sysvars; invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let mut syscall = SyscallGetRentSysvar { invoke_context: Rc::new(RefCell::new(&mut invoke_context)), @@ -3863,7 +3864,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let address = bpf_loader_upgradeable::id(); @@ -3979,7 +3980,7 @@ mod tests { ); let mut invoke_context = InvokeContext::new_mock(&accounts, &[]); invoke_context - .push(&message, &message.instructions[0], &[0], None) + .push(&message, &message.instructions[0], &[0], &[]) .unwrap(); let cost = invoke_context .get_compute_budget() diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index fc2e530f32..e9fee49396 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -437,7 +437,7 @@ mod tests { &preparation.message, &preparation.message.instructions[0], &program_indices, - Some(&preparation.account_indices), + &preparation.account_indices, )?; super::process_instruction(1, &instruction.data, &mut invoke_context) } @@ -1084,7 +1084,7 @@ mod tests { &preparation.message, &preparation.message.instructions[0], &program_indices, - Some(&preparation.account_indices), + &preparation.account_indices, ) .unwrap(); assert_eq!( diff --git a/rbpf-cli/src/main.rs b/rbpf-cli/src/main.rs index 92a59c70a6..6311e033df 100644 --- a/rbpf-cli/src/main.rs +++ b/rbpf-cli/src/main.rs @@ -213,7 +213,7 @@ native machine code before execting it in the virtual machine.", &preparation.message, &preparation.message.instructions[0], &program_indices, - Some(&preparation.account_indices), + &preparation.account_indices, ) .unwrap(); let keyed_accounts = invoke_context.get_keyed_accounts().unwrap(); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 5d0a7e7211..debe01f3e9 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -104,31 +104,19 @@ impl MessageProcessor { invoke_context.instruction_recorder = Some(&instruction_recorders[instruction_index]); } - let result = invoke_context - .push(message, instruction, program_indices, None) - .and_then(|_| { - let pre_remaining_units = - invoke_context.get_compute_meter().borrow().get_remaining(); - let mut time = Measure::start("execute_instruction"); - - invoke_context.process_instruction(&instruction.data)?; - invoke_context.verify(message, instruction, program_indices)?; - - time.stop(); - let post_remaining_units = - invoke_context.get_compute_meter().borrow().get_remaining(); - timings.accumulate_program( - instruction.program_id(&message.account_keys), - time.as_us(), - pre_remaining_units - post_remaining_units, - ); - timings.accumulate(&invoke_context.timings); - Ok(()) - }) - .map_err(|err| TransactionError::InstructionError(instruction_index as u8, err)); - invoke_context.pop(); - - result?; + let pre_remaining_units = invoke_context.get_compute_meter().borrow().get_remaining(); + let mut time = Measure::start("execute_instruction"); + invoke_context + .process_instruction(message, instruction, program_indices, &[], &[]) + .map_err(|err| TransactionError::InstructionError(instruction_index as u8, err))?; + time.stop(); + let post_remaining_units = invoke_context.get_compute_meter().borrow().get_remaining(); + timings.accumulate_program( + instruction.program_id(&message.account_keys), + time.as_us(), + pre_remaining_units - post_remaining_units, + ); + timings.accumulate(&invoke_context.timings); } Ok(()) }