From b310d044ac5c2bb1c874d0cfe701411e4aef47be Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 27 Aug 2023 11:07:40 +0200 Subject: [PATCH] fix!: skip the null-hash when validating the index. This is needed for compatibility with `index.skipHash`, which may skip producing the hash at the end of the index file, just filling in the null-hash. --- gix-index/src/decode/mod.rs | 11 +++--- gix-index/src/file/init.rs | 11 +++--- gix-index/src/file/verify.rs | 33 +++++++++--------- .../fixtures/loose_index/skip_hash.git-index | Bin 0 -> 97 bytes gix-index/tests/index/file/read.rs | 22 ++++++++++++ 5 files changed, 50 insertions(+), 27 deletions(-) create mode 100644 gix-index/tests/fixtures/loose_index/skip_hash.git-index diff --git a/gix-index/src/decode/mod.rs b/gix-index/src/decode/mod.rs index 70a65a880b0..12c8c53e404 100644 --- a/gix-index/src/decode/mod.rs +++ b/gix-index/src/decode/mod.rs @@ -54,7 +54,7 @@ pub struct Options { impl State { /// Decode an index state from `data` and store `timestamp` in the resulting instance for pass-through, assuming `object_hash` - /// to be used through the file. + /// to be used through the file. Also return the stored hash over all bytes in `data` or `None` if none was written due to `index.skipHash`. pub fn from_bytes( data: &[u8], timestamp: FileTime, @@ -64,7 +64,7 @@ impl State { min_extension_block_in_bytes_for_threading, expected_checksum, }: Options, - ) -> Result<(Self, gix_hash::ObjectId), Error> { + ) -> Result<(Self, Option), Error> { let _span = gix_features::trace::detail!("gix_index::State::from_bytes()"); let (version, num_entries, post_header_data) = header::decode(data, object_hash)?; let start_of_extensions = extension::end_of_index_entry::decode(data, object_hash); @@ -214,10 +214,11 @@ impl State { } let checksum = gix_hash::ObjectId::from(data); - if let Some(expected_checksum) = expected_checksum { - if checksum != expected_checksum { + let checksum = (!checksum.is_null()).then_some(checksum); + if let Some((expected_checksum, actual_checksum)) = expected_checksum.zip(checksum) { + if actual_checksum != expected_checksum { return Err(Error::ChecksumMismatch { - actual_checksum: checksum, + actual_checksum, expected_checksum, }); } diff --git a/gix-index/src/file/init.rs b/gix-index/src/file/init.rs index a61c7cfff60..1448de3dd4a 100644 --- a/gix-index/src/file/init.rs +++ b/gix-index/src/file/init.rs @@ -45,6 +45,9 @@ impl File { } /// Open an index file at `path` with `options`, assuming `object_hash` is used throughout the file. + /// + /// Note that the verification of the file hash depends on `options`, and even then it's performed after the file was read and not + /// before it is read. That way, invalid files would see a more descriptive error message as we try to parse them. pub fn at(path: impl Into, object_hash: gix_hash::Kind, options: decode::Options) -> Result { let _span = gix_features::trace::detail!("gix_index::File::at()"); let path = path.into(); @@ -57,11 +60,7 @@ impl File { }; let (state, checksum) = State::from_bytes(&data, mtime, object_hash, options)?; - let mut file = File { - state, - path, - checksum: Some(checksum), - }; + let mut file = File { state, path, checksum }; if let Some(mut link) = file.link.take() { link.dissolve_into(&mut file, object_hash, options)?; } @@ -72,7 +71,7 @@ impl File { /// Consume `state` and pretend it was read from `path`, setting our checksum to `null`. /// /// `File` instances created like that should be written to disk to set the correct checksum via `[File::write()]`. - pub fn from_state(state: crate::State, path: impl Into) -> Self { + pub fn from_state(state: State, path: impl Into) -> Self { File { state, path: path.into(), diff --git a/gix-index/src/file/verify.rs b/gix-index/src/file/verify.rs index a9680845c0e..3890acd9524 100644 --- a/gix-index/src/file/verify.rs +++ b/gix-index/src/file/verify.rs @@ -14,8 +14,6 @@ mod error { actual: gix_hash::ObjectId, expected: gix_hash::ObjectId, }, - #[error("Checksum of in-memory index wasn't computed yet")] - NoChecksum, } } pub use error::Error; @@ -24,19 +22,22 @@ impl File { /// Verify the integrity of the index to assure its consistency. pub fn verify_integrity(&self) -> Result<(), Error> { let _span = gix_features::trace::coarse!("gix_index::File::verify_integrity()"); - let checksum = self.checksum.ok_or(Error::NoChecksum)?; - let num_bytes_to_hash = self.path.metadata()?.len() - checksum.as_bytes().len() as u64; - let should_interrupt = AtomicBool::new(false); - let actual = gix_features::hash::bytes_of_file( - &self.path, - num_bytes_to_hash as usize, - checksum.kind(), - &mut gix_features::progress::Discard, - &should_interrupt, - )?; - (actual == checksum).then_some(()).ok_or(Error::ChecksumMismatch { - actual, - expected: checksum, - }) + if let Some(checksum) = self.checksum { + let num_bytes_to_hash = self.path.metadata()?.len() - checksum.as_bytes().len() as u64; + let should_interrupt = AtomicBool::new(false); + let actual = gix_features::hash::bytes_of_file( + &self.path, + num_bytes_to_hash as usize, + checksum.kind(), + &mut gix_features::progress::Discard, + &should_interrupt, + )?; + (actual == checksum).then_some(()).ok_or(Error::ChecksumMismatch { + actual, + expected: checksum, + }) + } else { + Ok(()) + } } } diff --git a/gix-index/tests/fixtures/loose_index/skip_hash.git-index b/gix-index/tests/fixtures/loose_index/skip_hash.git-index new file mode 100644 index 0000000000000000000000000000000000000000..8a755bd4d6c6cedebf6877d9c8f6d07caf3c327d GIT binary patch literal 97 zcmZ?q402{*U|<4bhL9jvS0E+HV4z^Y<=qr}%;|LA&IJiiy?$$ literal 0 HcmV?d00001 diff --git a/gix-index/tests/index/file/read.rs b/gix-index/tests/index/file/read.rs index 5cbae8b7bc3..d6fd6b78757 100644 --- a/gix-index/tests/index/file/read.rs +++ b/gix-index/tests/index/file/read.rs @@ -75,6 +75,28 @@ fn v2_empty() { assert!(tree.name.is_empty()); assert!(tree.children.is_empty()); assert_eq!(tree.id, hex_to_id("4b825dc642cb6eb9a060e54bf8d69288fbee4904")); + assert_eq!( + file.checksum(), + Some(hex_to_id("72d53f787d86a932a25a8537cee236d81846a8f1")), + "checksums are read but not validated by default" + ); +} + +#[test] +fn v2_empty_skip_hash() { + let file = loose_file("skip_hash"); + assert_eq!(file.version(), Version::V2); + assert_eq!(file.entries().len(), 0); + let tree = file.tree().unwrap(); + assert_eq!(tree.num_entries.unwrap_or_default(), 0); + assert!(tree.name.is_empty()); + assert!(tree.children.is_empty()); + assert_eq!(tree.id, hex_to_id("4b825dc642cb6eb9a060e54bf8d69288fbee4904")); + assert_eq!( + file.checksum(), + None, + "unset checksums are represented in the type system" + ); } #[test]