From 382d05ec6995efbdc4a4957da47fd5d2bd9ef663 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Tue, 21 Nov 2023 00:12:05 -0800 Subject: [PATCH 1/7] [TieredStorage] Boundary check for accessing hot account meta --- accounts-db/src/tiered_storage/hot.rs | 42 ++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index c460d3b9b27587..51fe839ca282a6 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -282,6 +282,15 @@ impl HotStorageReader { account_offset: HotAccountOffset, ) -> TieredStorageResult<&HotAccountMeta> { let internal_account_offset = account_offset.offset(); + let boundary = (self.footer.index_block_offset as usize) + .saturating_sub(std::mem::size_of::()); + + if internal_account_offset > boundary { + return Err(TieredStorageError::OffsetOutOfBounds( + internal_account_offset, + boundary, + )); + } let (meta, _) = get_pod::(&self.mmap, internal_account_offset)?; Ok(meta) @@ -538,7 +547,7 @@ pub mod tests { .collect(); let account_offsets: Vec<_>; - let footer = TieredStorageFooter { + let mut footer = TieredStorageFooter { account_meta_format: AccountMetaFormat::Hot, account_entry_count: NUM_ACCOUNTS, ..TieredStorageFooter::default() @@ -557,6 +566,7 @@ pub mod tests { .collect(); // while the test only focuses on account metas, writing a footer // here is necessary to make it a valid tiered-storage file. + footer.index_block_offset = current_offset as u64; footer.write_footer_block(&file).unwrap(); } @@ -566,9 +576,39 @@ pub mod tests { let meta = hot_storage.get_account_meta_from_offset(*offset).unwrap(); assert_eq!(meta, expected_meta); } + assert_eq!(&footer, hot_storage.footer()); } + #[test] + fn test_get_acount_meta_from_offset_out_of_bounds() { + // Generate a new temp path that is guaranteed to NOT already have a file. + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir + .path() + .join("test_get_acount_meta_from_offset_out_of_bounds"); + + let footer = TieredStorageFooter { + account_meta_format: AccountMetaFormat::Hot, + index_block_offset: 160, + ..TieredStorageFooter::default() + }; + + { + let file = TieredStorageFile::new_writable(&path).unwrap(); + footer.write_footer_block(&file).unwrap(); + } + + let hot_storage = HotStorageReader::new_from_path(&path).unwrap(); + let offset = HotAccountOffset::new(footer.index_block_offset as usize).unwrap(); + // Read from index_block_offset, which offset doesn't belong to + // account blocks. Expect Err here. + assert!(matches!( + hot_storage.get_account_meta_from_offset(offset), + Err(TieredStorageError::OffsetOutOfBounds(_, _)), + )); + } + #[test] fn test_hot_storage_get_account_offset_and_address() { // Generate a new temp path that is guaranteed to NOT already have a file. From afabe9755232ccd1f38653588af59fe9f245696b Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 14 Dec 2023 11:01:08 -0800 Subject: [PATCH 2/7] Convert to assert! --- accounts-db/src/tiered_storage/hot.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index 51fe839ca282a6..1b390a2f5321a2 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -282,15 +282,10 @@ impl HotStorageReader { account_offset: HotAccountOffset, ) -> TieredStorageResult<&HotAccountMeta> { let internal_account_offset = account_offset.offset(); - let boundary = (self.footer.index_block_offset as usize) - .saturating_sub(std::mem::size_of::()); - - if internal_account_offset > boundary { - return Err(TieredStorageError::OffsetOutOfBounds( - internal_account_offset, - boundary, - )); - } + assert!( + internal_account_offset <= + (self.footer.index_block_offset as usize) + .saturating_sub(std::mem::size_of::())); let (meta, _) = get_pod::(&self.mmap, internal_account_offset)?; Ok(meta) @@ -581,6 +576,7 @@ pub mod tests { } #[test] + #[should_panic(expected = "assertion failed")] fn test_get_acount_meta_from_offset_out_of_bounds() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = TempDir::new().unwrap(); @@ -602,11 +598,8 @@ pub mod tests { let hot_storage = HotStorageReader::new_from_path(&path).unwrap(); let offset = HotAccountOffset::new(footer.index_block_offset as usize).unwrap(); // Read from index_block_offset, which offset doesn't belong to - // account blocks. Expect Err here. - assert!(matches!( - hot_storage.get_account_meta_from_offset(offset), - Err(TieredStorageError::OffsetOutOfBounds(_, _)), - )); + // account blocks. Expect assert failure here + hot_storage.get_account_meta_from_offset(offset).unwrap(); } #[test] From 09059901a1e157e21315f65010fda657a904f6e0 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 14 Dec 2023 11:04:44 -0800 Subject: [PATCH 3/7] * formatting --- accounts-db/src/tiered_storage/hot.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index 1b390a2f5321a2..9b5f2c346f599f 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -283,9 +283,10 @@ impl HotStorageReader { ) -> TieredStorageResult<&HotAccountMeta> { let internal_account_offset = account_offset.offset(); assert!( - internal_account_offset <= - (self.footer.index_block_offset as usize) - .saturating_sub(std::mem::size_of::())); + internal_account_offset + <= (self.footer.index_block_offset as usize) + .saturating_sub(std::mem::size_of::()) + ); let (meta, _) = get_pod::(&self.mmap, internal_account_offset)?; Ok(meta) From ab898554f4d80674b5cbdaa7e53b5af056807c30 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Thu, 14 Dec 2023 23:28:30 -0800 Subject: [PATCH 4/7] Improve assert! and its test --- accounts-db/src/tiered_storage/hot.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index 9b5f2c346f599f..f3740aea9158ee 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -281,14 +281,13 @@ impl HotStorageReader { &self, account_offset: HotAccountOffset, ) -> TieredStorageResult<&HotAccountMeta> { - let internal_account_offset = account_offset.offset(); + let offset = account_offset.offset(); + assert!( - internal_account_offset - <= (self.footer.index_block_offset as usize) - .saturating_sub(std::mem::size_of::()) + offset.saturating_add(std::mem::size_of::()) + <= self.footer.index_block_offset as usize ); - - let (meta, _) = get_pod::(&self.mmap, internal_account_offset)?; + let (meta, _) = get_pod::(&self.mmap, offset)?; Ok(meta) } @@ -577,7 +576,9 @@ pub mod tests { } #[test] - #[should_panic(expected = "assertion failed")] + #[should_panic( + expected = "offset.saturating_add(std::mem::size_of::()) <=\\n self.footer.index_block_offset" + )] fn test_get_acount_meta_from_offset_out_of_bounds() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = TempDir::new().unwrap(); From 331299eeb1ef3dbe87cad966b76061640316ad7b Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Fri, 15 Dec 2023 10:08:56 -0800 Subject: [PATCH 5/7] Improve assert! and its test --- accounts-db/src/tiered_storage/hot.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index f3740aea9158ee..caf4aa82e7e862 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -576,9 +576,7 @@ pub mod tests { } #[test] - #[should_panic( - expected = "offset.saturating_add(std::mem::size_of::()) <=\\n self.footer.index_block_offset" - )] + #[should_panic(expected = "self.footer.index_block_offset")] fn test_get_acount_meta_from_offset_out_of_bounds() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = TempDir::new().unwrap(); From 4e98be507b2c88c6b91329b5c5ce7dfbbce4965a Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Sun, 17 Dec 2023 23:09:47 -0800 Subject: [PATCH 6/7] Improve assert! and test with custom message. --- accounts-db/src/tiered_storage/hot.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index caf4aa82e7e862..5709fc8a448f55 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -285,7 +285,10 @@ impl HotStorageReader { assert!( offset.saturating_add(std::mem::size_of::()) - <= self.footer.index_block_offset as usize + <= self.footer.index_block_offset as usize, + "HotAccountOffset ({}) exceeds accounts blocks offset boundary ({}).", + offset.saturating_add(std::mem::size_of::()), + self.footer.index_block_offset, ); let (meta, _) = get_pod::(&self.mmap, offset)?; Ok(meta) @@ -576,7 +579,7 @@ pub mod tests { } #[test] - #[should_panic(expected = "self.footer.index_block_offset")] + #[should_panic(expected = "exceeds accounts blocks offset boundary")] fn test_get_acount_meta_from_offset_out_of_bounds() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = TempDir::new().unwrap(); From 10431cc951a1e5594cb01d0735c1d47b141eb375 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Mon, 18 Dec 2023 10:11:26 -0800 Subject: [PATCH 7/7] Improving assert! message. --- accounts-db/src/tiered_storage/hot.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index 5709fc8a448f55..9a686736f9b178 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -286,8 +286,8 @@ impl HotStorageReader { assert!( offset.saturating_add(std::mem::size_of::()) <= self.footer.index_block_offset as usize, - "HotAccountOffset ({}) exceeds accounts blocks offset boundary ({}).", - offset.saturating_add(std::mem::size_of::()), + "reading HotAccountOffset ({}) would exceed accounts blocks offset boundary ({}).", + offset, self.footer.index_block_offset, ); let (meta, _) = get_pod::(&self.mmap, offset)?; @@ -579,7 +579,7 @@ pub mod tests { } #[test] - #[should_panic(expected = "exceeds accounts blocks offset boundary")] + #[should_panic(expected = "would exceed accounts blocks offset boundary")] fn test_get_acount_meta_from_offset_out_of_bounds() { // Generate a new temp path that is guaranteed to NOT already have a file. let temp_dir = TempDir::new().unwrap();