[TieredStorage] Boundary check for accessing hot account meta

[yhchiang-sol]

Problem

Hot accounts are stored in accounts blocks, whose offset is smaller than
the index block offset. However, the current code doesn't perform
any boundary checks when accessing hot account meta.

Summary of Changes

Adds boundary check when accessing hot account meta.

[codecov]

Codecov Report

Merging #34349 (10431cc) into master (1ac017c) will decrease coverage by 0.1%.
Report is 1 commits behind head on master.
The diff coverage is 100.0%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #34349     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         820      820             
  Lines      220869   220886     +17     
=========================================
+ Hits       180788   180800     +12     
- Misses      40081    40086      +5     

[brooksprumo]

Instead of checking the offsets when getting the account meta, can instead we enforce an invariant that the index offset is <= the account offset?

Mostly I'm wondering if this can be a programmer bug (aka assert), or if it can be malicious hot account files (aka check bounds and return error). I'm thinking programmer bug because we can set a maximum number of entries to read from the index, right?

[yhchiang-sol]

Instead of checking the offsets when getting the account meta, can instead we enforce an invariant that the index offset is <= the account offset?

Mostly I'm wondering if this can be a programmer bug (aka assert), or if it can be malicious hot account files (aka check bounds and return error). I'm thinking programmer bug because we can set a maximum number of entries to read from the index, right?

I think both make sense actually in different scenarios. The current implementation follows what AppendVec does where it has OffsetOutOfBounds error and UnableToLoad (from account_matches_owner API):

 #[derive(Error, Debug)]
 /// An enum for AppendVec related errors.
 pub enum AppendVecError {
     #[error("too small file size {0} for AppendVec")]
     FileSizeTooSmall(usize),

     #[error("too large file size {0} for AppendVec")]
     FileSizeTooLarge(usize),

     #[error("incorrect layout/length/data in the appendvec at path {}", .0.display())]
     IncorrectLayout(PathBuf),

     #[error("offset ({0}) is larger than file size ({1})")]
     OffsetOutOfBounds(usize, usize),
 }

What do you think?

[brooksprumo]

What do you think?

That's a bummer if we have to do this check, since this is part of the hot path.

Is it valid to pass in an offset that's too large though? Still feels like a programmer bug to me...

[yhchiang-sol]

That's a bummer if we have to do this check, since this is part of the hot path.
Is it valid to pass in an offset that's too large though? Still feels like a programmer bug to me.

It's not valid. Imo it's best to use just assert! if we don't need to make sure that calls from the program-rumtime or something can never cause panic. But it's that really the case since I found account_matches_owners() API might also UnableToLoad error instead of panic?

[yhchiang-sol]

Offline discussed with @brooksprumo. We will go with the assert! approach.

[yhchiang-sol]

Let me rebase to resolve the conflicts.

[brooksprumo]

Code looks good. Just a remaining question about the assert message.

[yhchiang-sol]

The solana/sanity failure looks odd. Will probably try rebase.

Checking remote `origin` for updates to target branch `master`
--
  | ++git diff origin/master --check --oneline
  | docs/publish-docs.sh:48: trailing whitespace.
  | +
  | docs/publish-docs.sh:66: trailing whitespace.
  | +
  | docs/publish-docs.sh:70: trailing whitespace.
  | +
  | docs/publish-docs.sh:81: trailing whitespace.
  | +
  | docs/publish-docs.sh:90: trailing whitespace.
  | +
  | docs/publish-docs.sh:99: trailing whitespace.
  | +
  | docs/publish-docs.sh:107: trailing whitespace.
  | +
  | docs/src/operations/setup-a-validator.md:9: trailing whitespace.
  | +Once you have a working validator on testnet, you will want to learn about [operational best practices](./best-practices/general.md) in the next section. Although the guide is specific to testnet, it can be adapted to mainnet or devnet as well.
  | >🚨 Error: The command exited with status 2

[brooksprumo]

The solana/sanity failure looks odd. Will probably try rebase.

Yeah, I'd rebase.

[yhchiang-sol]

Rebased on top of master.

[brooksprumo]

lgtm