Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TieredStorage] Boundary check for get_account_offset() #34531

Merged

Conversation

yhchiang-sol
Copy link
Contributor

Problem

TieredStorage doesn't perform boundary check in get_account_offset
when the input IndexOffset isn't valid.

Summary of Changes

This PR adds two checks. First, it checks whether the IndexOffset exceeds
the boundary of the index block. Second, when an index format that has the
same index entries as account entries is used, it also checks whether IndexOffset
is smaller than account_entry_count.

Test Plan

Two new tests are added in this PR.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cc0e5f7) 81.8% compared to head (da602bd) 81.8%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34531     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         822      822             
  Lines      221540   221607     +67     
=========================================
- Hits       181403   181395      -8     
- Misses      40137    40212     +75     

brooksprumo
brooksprumo previously approved these changes Dec 20, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Feel free to either fix the nit here, or merge as-is and fix all the nits together in a subsequent PR (so you don't have to wait through CI again).

@@ -83,13 +83,13 @@ impl IndexBlockFormat {
footer: &TieredStorageFooter,
index_offset: IndexOffset,
) -> TieredStorageResult<&'a Pubkey> {
let account_offset = match self {
let offset = match self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR probably shouldn't modify anything in get_account_address(), right? That's being handled here: #34529. Once that PR is merged, I'm not sure if there will be file conflicts here or not. I guess we'll see!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think this PR will have conflicts anyway as their tests are added in the same line. I am just trying to make the PR self-contained. Will do a rebase as #34529 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

debug_assert!(
offset.saturating_add(std::mem::size_of::<Offset>())
<= footer.owners_block_offset as usize,
"reading IndexOffset ({}) would exceeds index block boundary ({}).",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit here:

Suggested change
"reading IndexOffset ({}) would exceeds index block boundary ({}).",
"reading IndexOffset ({}) would exceed index block boundary ({}).",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix this one in the same PR as this PR needs a rebase anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

.open(&path)
.unwrap();
let mmap = unsafe { MmapOptions::new().map(&file).unwrap() };
// IndexOffset does not exceeds the account_entry_count but exceeds
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #34546

};

{
// we only writes a footer here as the test should hit an assert
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #34546.

@yhchiang-sol yhchiang-sol merged commit a583035 into solana-labs:master Dec 21, 2023
34 checks passed
yhchiang-sol added a commit to yhchiang-sol/solana that referenced this pull request Feb 13, 2024
…labs#33964)

[TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033)
[TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031)
[TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047)
[TieredStorage] HotStorageReader::get_account_address (solana-labs#34032)
[TieredStorage] OwnersBlock (solana-labs#34052)
[TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053)
[TieredStorage] Define OwnerOffset as u32 (solana-labs#34105)
[TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106)
Refactors TieredStorageFile read/write methods (solana-labs#34147)
[TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133)
[TieredStorage] Make IndexOffset use u32 (solana-labs#34152)
Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187)
[TieredStorage] Make AccountOffset use u32 (solana-labs#34151)
[TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155)
[TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335)
[TieredStorage]  Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404)
[TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376)
[TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349)
[TieredStorage] boundary check for get_account_address() (solana-labs#34529)
Sanitizes tiered storage footer after reading from disk (solana-labs#34200)
Adds read/write/get_pod() fns to tiered storage (solana-labs#34415)
Uses consistent error types in tiered storage (solana-labs#34110)
[TieredStorage] Boundary check for get_account_offset() (solana-labs#34531)
[TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350)
[TieredStorage] Fix typos in index.rs (solana-labs#34546)
[TieredStorage] HotAccountsReader::get_account (solana-labs#34499)
[TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658)
[TieredStorage] HotStorageWriter::new() (solana-labs#34659)
[TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724)
[TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854)
[TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853)
[TieredStorage] writing hot account blocks and index blocks (solana-labs#34828)
[TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950)
[TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927)
[TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969)
[TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967)
[TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968)
[TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929)
[TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046)
[TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031)
[TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049)
[TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
yhchiang-sol added a commit to yhchiang-sol/solana that referenced this pull request Feb 13, 2024
…34531)

#### Problem
TieredStorage doesn't perform boundary check in get_account_offset
when the input IndexOffset isn't valid.

#### Summary of Changes
This PR adds two checks.  First, it checks whether the IndexOffset exceeds
the boundary of the index block.  Second, when an index format that has the
same index entries as account entries is used, it also checks whether IndexOffset
is smaller than account_entry_count.

#### Test Plan
Two new tests are added to this PR.
yhchiang-sol added a commit to yhchiang-sol/solana that referenced this pull request Feb 13, 2024
…labs#33964)

[TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033)
[TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031)
[TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047)
[TieredStorage] HotStorageReader::get_account_address (solana-labs#34032)
[TieredStorage] OwnersBlock (solana-labs#34052)
[TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053)
[TieredStorage] Define OwnerOffset as u32 (solana-labs#34105)
[TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106)
Refactors TieredStorageFile read/write methods (solana-labs#34147)
[TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133)
[TieredStorage] Make IndexOffset use u32 (solana-labs#34152)
Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187)
[TieredStorage] Make AccountOffset use u32 (solana-labs#34151)
[TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155)
[TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335)
[TieredStorage]  Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404)
[TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376)
[TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349)
[TieredStorage] boundary check for get_account_address() (solana-labs#34529)
Sanitizes tiered storage footer after reading from disk (solana-labs#34200)
Adds read/write/get_pod() fns to tiered storage (solana-labs#34415)
Uses consistent error types in tiered storage (solana-labs#34110)
[TieredStorage] Boundary check for get_account_offset() (solana-labs#34531)
[TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350)
[TieredStorage] Fix typos in index.rs (solana-labs#34546)
[TieredStorage] HotAccountsReader::get_account (solana-labs#34499)
[TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658)
[TieredStorage] HotStorageWriter::new() (solana-labs#34659)
[TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724)
[TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854)
[TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853)
[TieredStorage] writing hot account blocks and index blocks (solana-labs#34828)
[TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950)
[TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927)
[TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969)
[TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967)
[TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968)
[TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929)
[TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046)
[TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031)
[TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049)
[TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
yhchiang-sol added a commit to yhchiang-sol/solana that referenced this pull request Feb 18, 2024
…labs#33964)

[TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033)
[TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031)
[TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047)
[TieredStorage] HotStorageReader::get_account_address (solana-labs#34032)
[TieredStorage] OwnersBlock (solana-labs#34052)
[TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053)
[TieredStorage] Define OwnerOffset as u32 (solana-labs#34105)
[TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106)
Refactors TieredStorageFile read/write methods (solana-labs#34147)
[TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133)
[TieredStorage] Make IndexOffset use u32 (solana-labs#34152)
Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187)
[TieredStorage] Make AccountOffset use u32 (solana-labs#34151)
[TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155)
[TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335)
[TieredStorage]  Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404)
[TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376)
[TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349)
[TieredStorage] boundary check for get_account_address() (solana-labs#34529)
Sanitizes tiered storage footer after reading from disk (solana-labs#34200)
Adds read/write/get_pod() fns to tiered storage (solana-labs#34415)
Uses consistent error types in tiered storage (solana-labs#34110)
[TieredStorage] Boundary check for get_account_offset() (solana-labs#34531)
[TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350)
[TieredStorage] Fix typos in index.rs (solana-labs#34546)
[TieredStorage] HotAccountsReader::get_account (solana-labs#34499)
[TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658)
[TieredStorage] HotStorageWriter::new() (solana-labs#34659)
[TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724)
[TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854)
[TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853)
[TieredStorage] writing hot account blocks and index blocks (solana-labs#34828)
[TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950)
[TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927)
[TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969)
[TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967)
[TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968)
[TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929)
[TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046)
[TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031)
[TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049)
[TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
[TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143)
yhchiang-sol added a commit to yhchiang-sol/solana that referenced this pull request Mar 4, 2024
…labs#33964)

[TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033)
[TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031)
[TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047)
[TieredStorage] HotStorageReader::get_account_address (solana-labs#34032)
[TieredStorage] OwnersBlock (solana-labs#34052)
[TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053)
[TieredStorage] Define OwnerOffset as u32 (solana-labs#34105)
[TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106)
Refactors TieredStorageFile read/write methods (solana-labs#34147)
[TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133)
[TieredStorage] Make IndexOffset use u32 (solana-labs#34152)
Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187)
[TieredStorage] Make AccountOffset use u32 (solana-labs#34151)
[TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155)
[TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335)
[TieredStorage]  Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404)
[TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376)
[TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349)
[TieredStorage] boundary check for get_account_address() (solana-labs#34529)
Sanitizes tiered storage footer after reading from disk (solana-labs#34200)
Adds read/write/get_pod() fns to tiered storage (solana-labs#34415)
Uses consistent error types in tiered storage (solana-labs#34110)
[TieredStorage] Boundary check for get_account_offset() (solana-labs#34531)
[TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350)
[TieredStorage] Fix typos in index.rs (solana-labs#34546)
[TieredStorage] HotAccountsReader::get_account (solana-labs#34499)
[TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658)
[TieredStorage] HotStorageWriter::new() (solana-labs#34659)
[TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724)
[TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854)
[TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853)
[TieredStorage] writing hot account blocks and index blocks (solana-labs#34828)
[TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950)
[TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927)
[TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969)
[TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967)
[TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968)
[TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929)
[TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046)
[TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031)
[TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049)
[TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
[TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143)
[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344)
yhchiang-sol added a commit to yhchiang-sol/solana that referenced this pull request Mar 9, 2024
…labs#33964)

[TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033)
[TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031)
[TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047)
[TieredStorage] HotStorageReader::get_account_address (solana-labs#34032)
[TieredStorage] OwnersBlock (solana-labs#34052)
[TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053)
[TieredStorage] Define OwnerOffset as u32 (solana-labs#34105)
[TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106)
Refactors TieredStorageFile read/write methods (solana-labs#34147)
[TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133)
[TieredStorage] Make IndexOffset use u32 (solana-labs#34152)
Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187)
[TieredStorage] Make AccountOffset use u32 (solana-labs#34151)
[TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155)
[TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335)
[TieredStorage]  Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404)
[TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376)
[TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349)
[TieredStorage] boundary check for get_account_address() (solana-labs#34529)
Sanitizes tiered storage footer after reading from disk (solana-labs#34200)
Adds read/write/get_pod() fns to tiered storage (solana-labs#34415)
Uses consistent error types in tiered storage (solana-labs#34110)
[TieredStorage] Boundary check for get_account_offset() (solana-labs#34531)
[TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350)
[TieredStorage] Fix typos in index.rs (solana-labs#34546)
[TieredStorage] HotAccountsReader::get_account (solana-labs#34499)
[TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658)
[TieredStorage] HotStorageWriter::new() (solana-labs#34659)
[TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724)
[TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854)
[TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853)
[TieredStorage] writing hot account blocks and index blocks (solana-labs#34828)
[TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950)
[TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927)
[TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969)
[TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967)
[TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968)
[TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929)
[TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046)
[TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031)
[TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049)
[TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
[TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143)
[TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344)
[TieredStorage] Deprecate the use of account-hash in HotStorage (solana-labs#93)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants