Ensure that account info address is not in an account

[seanyoung]

Problem

Summary of Changes

Fixes #

[mergify]

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

[Lichtso]

This should check the end of the slice, not the beginning.

[seanyoung]

True. This is also done in some other places, I'll fix those up too.

I suppose this would be hard to exploit but might as well do it right.

[seanyoung]

Actually I couldn't not find any others

[seanyoung]

So I've re-writtten this. It looks a bit like an open-coded llvm gep :) Let me know if there is a better way of doing this.

[Lichtso]

This check will become unreachable I think:

agave/programs/bpf_loader/src/syscalls/cpi.rs

Line 324 in 77789fc

// In the same vein as the other check_account_info_pointer() checks, we don't lock this

Because SolAccountInfo::data_len is not indirect, it is embedded in the array. Thus, if the entire array is checked in this PR, then the data len check in the C variant of the invoke syscall will be redundant.

[seanyoung]

This check will become unreachable I think:

agave/programs/bpf_loader/src/syscalls/cpi.rs

Line 324 in 77789fc

// In the same vein as the other check_account_info_pointer() checks, we don't lock this

Because SolAccountInfo::data_len is not indirect, it is embedded in the array. Thus, if the entire array is checked in this PR, then the data len check in the C variant of the invoke syscall will be redundant.

I agree, nice catch. Removed.

[alessandrod]

This check will become unreachable I think:

agave/programs/bpf_loader/src/syscalls/cpi.rs

Line 324 in 77789fc

// In the same vein as the other check_account_info_pointer() checks, we don't lock this

Because SolAccountInfo::data_len is not indirect, it is embedded in the array. Thus, if the entire array is checked in this PR, then the data len check in the C variant of the invoke syscall will be redundant.

I agree, nice catch. Removed.

how was the test not failing on this tho then if you were putting SolAccountInfos inside accounts?

[seanyoung]

how was the test not failing on this tho then if you were putting SolAccountInfos inside accounts?

The test is working on sol_invoke_signed_rust() which uses AccountInfo, sol_invoke_signed_c() uses SolAccountInfo.

Come to think it, let me check if we have coverage for that.

[seanyoung]

Come to think it, let me check if we have coverage for that.

I couldn't find an explicit test, so I've added one.

[seanyoung]

@Lichtso could I have another review please, thanks