You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dentiny (via GitHub)" <gi...@apache.org> on 2026/05/09 00:12:43 UTC
[PR] fix(puffin): add missing magic number validation for puffins [iceberg-rust]
dentiny opened a new pull request, #2416:
URL: https://github.com/apache/iceberg-rust/pull/2416
## What changes are included in this PR?
We have two functions to read puffin metadata: `read` and `read_with_prefetch`. In theory these two functions should be similarly the same apart from IO-related logic, but `read_with_prefetch` seems to miss magic number validation: https://github.com/apache/iceberg-rust/blob/4f21e503afc5363c10fafac917f0febe136c464e/crates/iceberg/src/puffin/metadata.rs#L284
## Are these changes tested?
Yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): add missing magic number validation for puffins [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2416:
URL: https://github.com/apache/iceberg-rust/pull/2416#discussion_r3232244908
##########
crates/iceberg/src/puffin/metadata.rs:
##########
@@ -324,7 +324,11 @@ impl FileMetadata {
return FileMetadata::read(input_file).await;
}
- // Read footer based on prefetchi hint
+ // Validate file header magic
+ let first_four_bytes = file_read.read(0..FileMetadata::MAGIC_LENGTH.into()).await?;
Review Comment:
Confirmed that this matches https://github.com/apache/iceberg-rust/pull/2416/changes#diff-767abff39c635c6cdb9182fa8e196ace14cf9598e14a8249db3639c35e162311R281-R284 and if `prefetch_hint` is less greater than the file length then we hit the regular read path which does this validation so this seems correct to me.
##########
crates/iceberg/src/puffin/metadata.rs:
##########
@@ -958,6 +962,33 @@ mod tests {
assert_eq!(file_metadata, zstd_compressed_metric_file_metadata());
}
+ #[tokio::test]
+ async fn test_read_with_incorrect_header_magic() {
+ let temp_dir = TempDir::new().unwrap();
+
+ let prefetch_hint: u8 = 64;
+ let mut bytes = vec![];
+ // Invalid header magic
+ bytes.extend([0x00, 0x00, 0x00, 0x00]);
+ // Intentionally keep file size larger than prefetch_hint.
+ bytes.extend(vec![0u8; prefetch_hint as usize]);
+ // Valid footer: magic + payload + footer struct
+ bytes.extend(FileMetadata::MAGIC);
+ bytes.extend(empty_footer_payload_bytes());
+ bytes.extend(empty_footer_payload_bytes_length_bytes());
+ bytes.extend(vec![0, 0, 0, 0]); // flags
+ bytes.extend(FileMetadata::MAGIC);
+
+ let input_file = input_file_with_bytes(&temp_dir, &bytes).await;
+
+ assert!(FileMetadata::read(&input_file).await.is_err(),);
+ assert!(
+ FileMetadata::read_with_prefetch(&input_file, prefetch_hint)
+ .await
+ .is_err(),
Review Comment:
Might be nice to assert on the actual error here rather asserting on any error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): add missing magic number validation for puffins [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2416:
URL: https://github.com/apache/iceberg-rust/pull/2416#issuecomment-4438365662
Thanks for the fix @dentiny. Left a comment on the test, let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): add missing magic number validation for puffins [iceberg-rust]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on code in PR #2416:
URL: https://github.com/apache/iceberg-rust/pull/2416#discussion_r3232356003
##########
crates/iceberg/src/puffin/metadata.rs:
##########
@@ -958,6 +962,33 @@ mod tests {
assert_eq!(file_metadata, zstd_compressed_metric_file_metadata());
}
+ #[tokio::test]
+ async fn test_read_with_incorrect_header_magic() {
+ let temp_dir = TempDir::new().unwrap();
+
+ let prefetch_hint: u8 = 64;
+ let mut bytes = vec![];
+ // Invalid header magic
+ bytes.extend([0x00, 0x00, 0x00, 0x00]);
+ // Intentionally keep file size larger than prefetch_hint.
+ bytes.extend(vec![0u8; prefetch_hint as usize]);
+ // Valid footer: magic + payload + footer struct
+ bytes.extend(FileMetadata::MAGIC);
+ bytes.extend(empty_footer_payload_bytes());
+ bytes.extend(empty_footer_payload_bytes_length_bytes());
+ bytes.extend(vec![0, 0, 0, 0]); // flags
+ bytes.extend(FileMetadata::MAGIC);
+
+ let input_file = input_file_with_bytes(&temp_dir, &bytes).await;
+
+ assert!(FileMetadata::read(&input_file).await.is_err(),);
+ assert!(
+ FileMetadata::read_with_prefetch(&input_file, prefetch_hint)
+ .await
+ .is_err(),
Review Comment:
sure, updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): add missing magic number validation for puffins [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY merged PR #2416:
URL: https://github.com/apache/iceberg-rust/pull/2416
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(puffin): add missing magic number validation for puffins [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2416:
URL: https://github.com/apache/iceberg-rust/pull/2416#issuecomment-4438551697
@CTTY this PR makes sense to me, would you be able to take a look when you have a minute?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org