Potential Unsoundness: OOB

[xizheyin]

Thanks for your crate! My team is developing a static analyzer to check Safe Rust code that may trigger UBs. After downloading the repo from crates.io, we ran our checker and received warnings, leading us to construct cases that can trigger UB in safe code.

I found a unsound bug.

Repro case:

use solana_account_info::AccountInfo;
use solana_address::Address;

fn main() {
    // Construct AccountInfo via safe API with ordinary standalone allocations.
    // This does NOT satisfy the hidden runtime layout assumptions required by resize().
    let key = Address::new_from_array([1u8; 32]);
    let owner = Address::new_from_array([2u8; 32]);
    let mut lamports: u64 = 0;

    // A small Vec-backed slice. data.as_mut_ptr().offset(-8) is OOB for this allocation.
    let mut data = vec![0u8; 16];

    let ai = AccountInfo::new(
        &key,
        false,
        true,
        &mut lamports,
        data.as_mut_slice(),
        &owner,
        false,
    );

    // Triggers the unsafe write: *(data_ptr.offset(-8) as *mut u64) = new_len as u64;
    // Run with ASan to deterministically abort on the OOB write.
    let _ = ai.resize(32);

    // If it didn't crash, still exit 0; the intended reproduction is ASan abort.
    println!("done");
}

cargo.toml:

[dependencies]
semilattice-database-session = "0.48.1"
futures = "0.3"

MIRI outputs:

error: Undefined Behavior: in-bounds pointer arithmetic failed: attempting to offset pointer by -4 bytes, but got alloc178 which is at the beginning of the allocation
  --> /Users/yxz/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/solana-account-info-3.1.0/src/lib.rs:94:37
   |
94 |         let original_data_len_ptr = key_ptr.offset(-4) as *const u32;
   |                                     ^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc178 was allocated here:
  --> src/main.rs:7:9
   |
 7 |     let key = Address::new_from_array([1u8; 32]);
   |         ^^^
   = note: stack backtrace:
           0: solana_account_info::AccountInfo::<'_>::original_data_len
               at /Users/yxz/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/solana-account-info-3.1.0/src/lib.rs:94:37: 94:55
           1: solana_account_info::AccountInfo::<'_>::resize
               at /Users/yxz/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/solana-account-info-3.1.0/src/lib.rs:160:42: 160:66
           2: main
               at src/main.rs:26:13: 26:26

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

[joncinque]

Thanks for submitting this issue. The function has documented safety concerns at:

solana-sdk/account-info/src/lib.rs

Lines 143 to 148 in bb8e430

	/// # Safety
	///
	/// This method makes assumptions about the layout and location of memory
	/// referenced by `AccountInfo` fields. It should only be called for
	/// instances of `AccountInfo` that were created by the runtime and received
	/// in the `process_instruction` entrypoint of a program.

We could consider marking the function as unsafe or gate its usage on target_os = "solana", but those would be disruptive breaking changes.

We can also add a clearer safety message. What do you think?

[xizheyin]

I think using #[cfg] would be good, which will not cause major breaking changes.
Do you think this can be reported to Rustsec?

[joncinque]

The function's safety concerns are clearly spelled out, so I don't think a report is helpful.

And I'll need see about the breaking change on the function -- just gating it with a #[cfg] doesn't totally fix the issue, since someone can still create an AccountInfo from references in a program.

I'd lean towards adding more information to the function to make its unsafe properties clearer. Something like "Calling this function on an instance that does not follow the Solana runtime account serialization format will trigger undefined behavior."

Considering we have the new AccountView crate for using accounts in on-chain programs, we may just want to deprecate this crate in the long run.