memory syscalls: improve test coverage and other review comments

programs/bpf_loader/src/syscalls/mem_ops.rs

@@ -490,18 +490,24 @@ impl<'a> Iterator for MemoryChunkIterator<'a> {
 
         let region_is_account;
 
-        let mut account_index = self.account_index.unwrap_or_default();
-        self.account_index = Some(account_index);
+        let account_index = self.account_index.get_or_insert_default();
 
+        // Do not allow iteration over account/non-account boundary.
+
+        // First check whether the region is account data or not
+        // Note: that if an account has empty account data, it still has a resize area
+        // Note: for deprecated unaligned programs, there is no resize area an empty data accounts do NOT count as accounts
         loop {
-            if let Some(account) = self.accounts.get(account_index) {
+            if let Some(account) = self.accounts.get(*account_index) {
+                // region is either account data or 10k resize region
                 let account_addr = account.vm_data_addr;
                 let resize_addr = account_addr.saturating_add(account.original_data_len as u64);
 
                 if resize_addr < region.vm_addr {
-                    // region is after this account, move on next one
-                    account_index = account_index.saturating_add(1);
-                    self.account_index = Some(account_index);
+                    // region is not this account, move onto the next account
+
+                    // note that we iterate in the same linear direction as regions
+                    *account_index = account_index.saturating_add(1);
                 } else {
                     region_is_account =
                         region.vm_addr == account_addr || region.vm_addr == resize_addr;
@@ -556,25 +562,31 @@ impl DoubleEndedIterator for MemoryChunkIterator<'_> {
 
         let region_is_account;
 
-        let mut account_index = self
+        let account_index = self
             .account_index
-            .unwrap_or_else(|| self.accounts.len().saturating_sub(1));
-        self.account_index = Some(account_index);
+            .get_or_insert_with(|| self.accounts.len().saturating_sub(1));
+
+        // Do not allow iteration over account/non-account boundary.
 
+        // First check whether the region is account data or not
+        // Note: that if an account has empty account data, it still has a resize area
+        // Note: for deprecated unaligned programs, there is no resize area an empty data accounts do NOT count as accounts
         loop {
-            let Some(account) = self.accounts.get(account_index) else {
+            let Some(account) = self.accounts.get(*account_index) else {
                 // address is after all the accounts
                 region_is_account = false;
                 break;
             };
 
+            // region is either account data or 10k resize region
             let account_addr = account.vm_data_addr;
             let resize_addr = account_addr.saturating_add(account.original_data_len as u64);
 
-            if account_index > 0 && account_addr > region.vm_addr {
-                account_index = account_index.saturating_sub(1);
+            if *account_index > 0 && account_addr > region.vm_addr {
+                // region is not this account, move onto the previous account
 
-                self.account_index = Some(account_index);
+                // note that we iterate in the same linear direction as regions
+                *account_index = account_index.saturating_sub(1);
             } else {
                 region_is_account = region.vm_addr == account_addr || region.vm_addr == resize_addr;
                 break;

programs/sbf/rust/account_mem/src/lib.rs

@@ -105,7 +105,7 @@ pub fn process_instruction(
             sol_memcpy(too_early(3), &buf, 10);
         }
         13 => {
-            // memmov dst overlaps begin of account
+            // memmove dst overlaps begin of account
             unsafe { sol_memmove(too_early(3).as_mut_ptr(), buf.as_ptr(), 10) };
         }
         14 => {
@@ -122,6 +122,55 @@ pub fn process_instruction(
                 )
             };
         }
+        16 => {
+            // memmove at the lower boundary fails (by 1) - reverse
+            unsafe { sol_memmove(data.as_mut_ptr(), data.as_ptr().wrapping_sub(1), 100) };
+        }
+        17 => {
+            // memmove at the lower boundary fails (by 1) - forward
+            unsafe { sol_memmove(data.as_mut_ptr().wrapping_sub(1), data.as_ptr(), 100) };
+        }
+        18 => {
+            // memmove at the higher account boundary fails (by 1) - forward
+            unsafe {
+                sol_memmove(
+                    data.as_mut_ptr().wrapping_add(data_len.wrapping_sub(100)),
+                    data.as_ptr().wrapping_add(data_len.wrapping_sub(99)),
+                    100,
+                )
+            };
+        }
+        19 => {
+            // memmove at the higher account boundary fails (by 1) - reverse
+            unsafe {
+                sol_memmove(
+                    data.as_mut_ptr().wrapping_add(data_len.wrapping_sub(99)),
+                    data.as_ptr().wrapping_add(data_len.wrapping_sub(100)),
+                    100,
+                )
+            };
+        }
+        100 => {
+            // memmove at the lower boundary succeeds
+            unsafe { sol_memmove(data.as_mut_ptr().wrapping_add(50), data.as_ptr(), 100) };
+
+            // memmove at the higher boundary succeeds
+            unsafe {
+                sol_memmove(
+                    data.as_mut_ptr().wrapping_add(data_len.wrapping_sub(101)),
+                    data.as_ptr().wrapping_sub(data_len.wrapping_sub(100)),
+                    100,
+                )
+            };
+
+            unsafe {
+                sol_memmove(
+                    data.as_mut_ptr().wrapping_add(data_len.wrapping_sub(100)),
+                    data.as_ptr().wrapping_sub(data_len.wrapping_sub(101)),
+                    100,
+                )
+            };
+        }
         _ => {}
     }
 

programs/sbf/tests/programs.rs

@@ -5179,7 +5179,7 @@ fn test_mem_syscalls_overlap_account_begin_or_end() {
         let account = AccountSharedData::new(42, 1024, &program_id);
         bank.store_account(&account_keypair.pubkey(), &account);
 
-        for instr in 0..=15 {
+        for instr in 0..=19 {
             println!("Testing direct_mapping:{direct_mapping} instruction:{instr}");
             let instruction =
                 Instruction::new_with_bytes(program_id, &[instr], account_metas.clone());
@@ -5195,5 +5195,15 @@ fn test_mem_syscalls_overlap_account_begin_or_end() {
                 assert!(!logs.last().unwrap().ends_with(" failed: InvalidLength"));
             }
         }
+
+        // test success
+        println!("Testing direct_mapping:{direct_mapping} success");
+        let instruction = Instruction::new_with_bytes(program_id, &[199], account_metas.clone());
+
+        let message = Message::new(&[instruction], Some(&mint_pubkey));
+        let tx = Transaction::new(&[&mint_keypair], message.clone(), bank.last_blockhash());
+        let (result, _, logs, _) = process_transaction_and_record_inner(&bank, tx);
+
+        assert!(result.is_ok(), "{logs:?}");
     }
 }