From b17efc70c8d3e7d88213f8d55447f98603626335 Mon Sep 17 00:00:00 2001 From: Polly Labs Date: Tue, 28 Jul 2026 14:48:27 -0500 Subject: [PATCH 1/2] fix(delta): return errors for malformed streams --- src/delta/decode/mod.rs | 86 +++++++++++++++++++++++++++++++++-------- src/delta/utils.rs | 2 +- 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/delta/decode/mod.rs b/src/delta/decode/mod.rs index cb485a5b..c10929b7 100644 --- a/src/delta/decode/mod.rs +++ b/src/delta/decode/mod.rs @@ -10,6 +10,10 @@ const COPY_OFFSET_BYTES: u8 = 4; const COPY_SIZE_BYTES: u8 = 3; const COPY_ZERO_SIZE: usize = 0x10000; +fn decoder_error(message: impl Into) -> GitDeltaError { + GitDeltaError::DeltaDecoderError(message.into()) +} + /// Apply a delta stream to `base_info`, returning the reconstructed target bytes. /// The stream format matches Git's delta encoding (see `delta::encode`): /// - leading base size, then result size (varint) @@ -20,14 +24,14 @@ pub fn delta_decode( base_info: &[u8], ) -> Result, GitDeltaError> { // Read declared base size and result size - let base_size = utils::read_size_encoding(&mut stream).unwrap(); + let base_size = + utils::read_size_encoding(&mut stream).map_err(|err| decoder_error(err.to_string()))?; if base_info.len() != base_size { - return Err(GitDeltaError::DeltaDecoderError( - "base object len is not equal".to_owned(), - )); + return Err(decoder_error("base object len is not equal")); } - let result_size = utils::read_size_encoding(&mut stream).unwrap(); + let result_size = + utils::read_size_encoding(&mut stream).map_err(|err| decoder_error(err.to_string()))?; let mut buffer = Vec::with_capacity(result_size); loop { // Check if the stream has ended, meaning the new object is done @@ -35,7 +39,7 @@ pub fn delta_decode( Ok([instruction]) => instruction, Err(err) if err.kind() == ErrorKind::UnexpectedEof => break, Err(err) => { - panic!("{}", format!("Wrong instruction in delta :{err}")); + return Err(decoder_error(format!("Wrong instruction in delta: {err}"))); } }; @@ -43,15 +47,14 @@ pub fn delta_decode( // Data instruction; the instruction byte specifies the number of data bytes if instruction == 0 { // Appending 0 bytes doesn't make sense, so git disallows it - panic!( - "{}", - GitDeltaError::DeltaDecoderError(String::from("Invalid data instruction")) - ); + return Err(decoder_error("Invalid data instruction")); } // Append the provided bytes let mut data = vec![0; instruction as usize]; - stream.read_exact(&mut data).unwrap(); + stream + .read_exact(&mut data) + .map_err(|err| decoder_error(err.to_string()))?; buffer.extend_from_slice(&data); // result.extend_from_slice(&data); } else { @@ -59,22 +62,31 @@ pub fn delta_decode( let mut nonzero_bytes = instruction; let offset = utils::read_partial_int(&mut stream, COPY_OFFSET_BYTES, &mut nonzero_bytes) - .unwrap(); + .map_err(|err| decoder_error(err.to_string()))?; let mut size = - utils::read_partial_int(&mut stream, COPY_SIZE_BYTES, &mut nonzero_bytes).unwrap(); + utils::read_partial_int(&mut stream, COPY_SIZE_BYTES, &mut nonzero_bytes) + .map_err(|err| decoder_error(err.to_string()))?; if size == 0 { // Copying 0 bytes doesn't make sense, so git assumes a different size size = COPY_ZERO_SIZE; } // Copy bytes from the base object - let base_data = base_info.get(offset..(offset + size)).ok_or_else(|| { - GitDeltaError::DeltaDecoderError("Invalid copy instruction".to_string()) - }); + let end = offset + .checked_add(size) + .ok_or_else(|| decoder_error("Invalid copy instruction"))?; + let base_data = base_info + .get(offset..end) + .ok_or_else(|| decoder_error("Invalid copy instruction")); buffer.extend_from_slice(base_data?); } } - assert!(buffer.len() == result_size); + if buffer.len() != result_size { + return Err(decoder_error(format!( + "result object len is not equal: expected {result_size}, got {}", + buffer.len() + ))); + } Ok(buffer) } @@ -109,4 +121,44 @@ mod tests { let err = delta_decode(&mut cursor, b"xx").unwrap_err(); assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); } + + /// Truncated delta headers should return an error instead of panicking. + #[test] + fn truncated_header_returns_error() { + let mut cursor = Cursor::new(Vec::::new()); + let err = delta_decode(&mut cursor, b"").unwrap_err(); + assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); + } + + /// Git disallows a zero-length literal instruction; report it as malformed input. + #[test] + fn zero_literal_instruction_returns_error() { + let mut cursor = Cursor::new(vec![0, 0, 0]); + let err = delta_decode(&mut cursor, b"").unwrap_err(); + assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); + } + + /// Literal instructions whose payload is shorter than declared should return an error. + #[test] + fn truncated_literal_instruction_returns_error() { + let mut cursor = Cursor::new(vec![0, 3, 3, b'a']); + let err = delta_decode(&mut cursor, b"").unwrap_err(); + assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); + } + + /// Copy instructions whose operand bytes are missing should return an error. + #[test] + fn truncated_copy_instruction_returns_error() { + let mut cursor = Cursor::new(vec![3, 1, 0x81]); + let err = delta_decode(&mut cursor, b"abc").unwrap_err(); + assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); + } + + /// Deltas that end before producing the declared result size should return an error. + #[test] + fn result_size_mismatch_returns_error() { + let mut cursor = Cursor::new(vec![0, 1]); + let err = delta_decode(&mut cursor, b"").unwrap_err(); + assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); + } } diff --git a/src/delta/utils.rs b/src/delta/utils.rs index e91cc5e7..d1b53661 100644 --- a/src/delta/utils.rs +++ b/src/delta/utils.rs @@ -21,7 +21,7 @@ pub fn read_size_encoding(stream: &mut R) -> std::io::Result { let mut length = 0; loop { - let (byte_value, more_bytes) = read_var_int_byte(stream).unwrap(); + let (byte_value, more_bytes) = read_var_int_byte(stream)?; value |= (byte_value as usize) << length; if !more_bytes { return Ok(value); From 081bde72cf78123d05bb51184a19943d84d857e2 Mon Sep 17 00:00:00 2001 From: Quanyi Ma Date: Wed, 29 Jul 2026 13:53:19 +0800 Subject: [PATCH 2/2] fix(delta): harden decoder and plug pack rebuild path - reject overlong size varints with InvalidData instead of shift overflow - reserve result buffer incrementally and cap output at declared result size - reuse delta_decode in Pack::rebuild_delta_with_hash and return GitError - propagate delta rebuild failures from thread pool through SharedParams - add regression tests for varint overflow, unallocatable sizes, output-size overruns, and malformed pack delta objects --- src/delta/decode/mod.rs | 72 +++++++++++++- src/delta/mod.rs | 2 + src/delta/utils.rs | 13 ++- src/internal/pack/decode.rs | 190 +++++++++++++++++++----------------- 4 files changed, 184 insertions(+), 93 deletions(-) diff --git a/src/delta/decode/mod.rs b/src/delta/decode/mod.rs index c10929b7..0af90e6a 100644 --- a/src/delta/decode/mod.rs +++ b/src/delta/decode/mod.rs @@ -32,7 +32,7 @@ pub fn delta_decode( let result_size = utils::read_size_encoding(&mut stream).map_err(|err| decoder_error(err.to_string()))?; - let mut buffer = Vec::with_capacity(result_size); + let mut buffer = Vec::new(); loop { // Check if the stream has ended, meaning the new object is done let instruction = match utils::read_bytes(stream) { @@ -55,6 +55,7 @@ pub fn delta_decode( stream .read_exact(&mut data) .map_err(|err| decoder_error(err.to_string()))?; + prepare_result_append(&mut buffer, data.len(), result_size)?; buffer.extend_from_slice(&data); // result.extend_from_slice(&data); } else { @@ -78,7 +79,9 @@ pub fn delta_decode( .get(offset..end) .ok_or_else(|| decoder_error("Invalid copy instruction")); - buffer.extend_from_slice(base_data?); + let base_data = base_data?; + prepare_result_append(&mut buffer, base_data.len(), result_size)?; + buffer.extend_from_slice(base_data); } } if buffer.len() != result_size { @@ -90,6 +93,26 @@ pub fn delta_decode( Ok(buffer) } +fn prepare_result_append( + buffer: &mut Vec, + instruction_size: usize, + result_size: usize, +) -> Result<(), GitDeltaError> { + let new_size = buffer + .len() + .checked_add(instruction_size) + .ok_or_else(|| decoder_error("result object size overflow"))?; + if new_size > result_size { + return Err(decoder_error(format!( + "result object exceeds declared size: expected {result_size}, got at least {new_size}" + ))); + } + buffer + .try_reserve_exact(instruction_size) + .map_err(|err| decoder_error(format!("cannot allocate result object: {err}")))?; + Ok(()) +} + #[cfg(test)] mod tests { use std::io::Cursor; @@ -161,4 +184,49 @@ mod tests { let err = delta_decode(&mut cursor, b"").unwrap_err(); assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); } + + /// Size varints wider than usize should be rejected instead of overflowing a shift. + #[test] + fn overlong_size_varint_returns_error() { + let mut bytes = vec![0x80; 10]; + bytes.push(0); + let mut cursor = Cursor::new(bytes); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + delta_decode(&mut cursor, b"") + })); + + assert!(result.is_ok(), "overlong size varint should not panic"); + assert!(matches!( + result.unwrap(), + Err(GitDeltaError::DeltaDecoderError(_)) + )); + } + + /// An unallocatable declared result size should be rejected without reserving it eagerly. + #[test] + fn unallocatable_result_size_returns_error() { + let mut bytes = vec![0]; + bytes.extend([0xff; 9]); + bytes.push(1); + let mut cursor = Cursor::new(bytes); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + delta_decode(&mut cursor, b"") + })); + + assert!(result.is_ok(), "unallocatable result size should not panic"); + assert!(matches!( + result.unwrap(), + Err(GitDeltaError::DeltaDecoderError(_)) + )); + } + + /// Instructions may not produce more bytes than the declared result size. + #[test] + fn instruction_exceeding_result_size_returns_error() { + let mut cursor = Cursor::new(vec![0, 1, 2, b'a', b'b']); + let err = delta_decode(&mut cursor, b"").unwrap_err(); + assert!(matches!(err, GitDeltaError::DeltaDecoderError(_))); + } } diff --git a/src/delta/mod.rs b/src/delta/mod.rs index 7c3a60a5..bdb53480 100644 --- a/src/delta/mod.rs +++ b/src/delta/mod.rs @@ -13,6 +13,8 @@ mod encode; mod errors; mod utils; +pub(crate) use decode::delta_decode; + const SAMPLE_STEP: usize = 64; const MIN_DELTA_RATE: f64 = 0.5; diff --git a/src/delta/utils.rs b/src/delta/utils.rs index d1b53661..223d7cce 100644 --- a/src/delta/utils.rs +++ b/src/delta/utils.rs @@ -1,7 +1,7 @@ //! Shared readers for Git delta streams: length parsing, partial integer decoding, and VarInt helpers //! that both encoder and decoder reuse. -use std::io::Read; +use std::io::{self, Read}; const VAR_INT_ENCODING_BITS: u8 = 7; const VAR_INT_CONTINUE_FLAG: u8 = 1 << VAR_INT_ENCODING_BITS; @@ -22,12 +22,21 @@ pub fn read_size_encoding(stream: &mut R) -> std::io::Result { loop { let (byte_value, more_bytes) = read_var_int_byte(stream)?; + if length >= usize::BITS + || (length + u32::from(VAR_INT_ENCODING_BITS) > usize::BITS + && (byte_value as usize) > (usize::MAX >> length)) + { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "delta size varint exceeds usize", + )); + } value |= (byte_value as usize) << length; if !more_bytes { return Ok(value); } - length += VAR_INT_ENCODING_BITS; + length += u32::from(VAR_INT_ENCODING_BITS); } } diff --git a/src/internal/pack/decode.rs b/src/internal/pack/decode.rs index fbc0d9eb..4a07e3a4 100644 --- a/src/internal/pack/decode.rs +++ b/src/internal/pack/decode.rs @@ -7,10 +7,10 @@ use std::io::{Seek, SeekFrom}; use std::os::unix::fs::FileExt; use std::{ fs::File, - io::{self, BufRead, Cursor, ErrorKind, Read, Write}, + io::{self, BufRead, Cursor, Read, Write}, path::{Path, PathBuf}, sync::{ - Arc, OnceLock, + Arc, Mutex, OnceLock, atomic::{AtomicUsize, Ordering}, }, thread::{self, JoinHandle}, @@ -137,6 +137,7 @@ struct SharedParams { pub callback: Option, pub retention: Option>, pub skip_unneeded_objects: bool, + pub error: Mutex>, } #[derive(Default)] @@ -1813,6 +1814,7 @@ impl Pack { callback, retention: retention_mode.retention, skip_unneeded_objects: retention_mode.skip_unneeded_objects, + error: Mutex::new(None), }); let mut reader = if verify_pack_stream_hash { Wrapper::new(pack) @@ -1969,6 +1971,11 @@ impl Pack { self.pool.join(); // wait for all threads to finish + if let Some(error) = shared_params.error.lock().unwrap().take() { + self.abort_decode(); + return Err(error); + } + // send pack id for metadata if let Some(pack_callback) = pack_id_callback { pack_callback(self.signature); @@ -2257,16 +2264,27 @@ impl Pack { shared_params.pool.clone().execute(move || { let known_hash = delta_obj.known_hash; - let mut new_obj = match delta_obj.info { + let new_obj = match delta_obj.info { CacheObjectInfo::OffsetDelta(_, _) | CacheObjectInfo::HashDelta(_, _) => { Pack::rebuild_delta_with_hash(delta_obj, base_obj, known_hash) } - CacheObjectInfo::OffsetZstdelta(_, _) => { - Pack::rebuild_zstdelta_with_hash(delta_obj, base_obj, known_hash) - } + CacheObjectInfo::OffsetZstdelta(_, _) => Ok(Pack::rebuild_zstdelta_with_hash( + delta_obj, base_obj, known_hash, + )), _ => unreachable!(), }; + let mut new_obj = match new_obj { + Ok(new_obj) => new_obj, + Err(error) => { + let mut decode_error = shared_params.error.lock().unwrap(); + if decode_error.is_none() { + *decode_error = Some(error); + } + return; + } + }; + new_obj.set_mem_recorder(shared_params.cache_objs_mem_size.clone()); new_obj.record_mem_size(); Self::cache_obj_and_process_waitlist(&shared_params, new_obj); //Indirect Recursion @@ -2371,7 +2389,10 @@ impl Pack { /// Reconstruct the Delta Object based on the "base object" /// and return the new object. - pub fn rebuild_delta(delta_obj: CacheObject, base_obj: Arc) -> CacheObject { + pub fn rebuild_delta( + delta_obj: CacheObject, + base_obj: Arc, + ) -> Result { Self::rebuild_delta_with_hash(delta_obj, base_obj, None) } @@ -2379,91 +2400,15 @@ impl Pack { delta_obj: CacheObject, base_obj: Arc, known_hash: Option, - ) -> CacheObject { - const COPY_INSTRUCTION_FLAG: u8 = 1 << 7; - const COPY_OFFSET_BYTES: u8 = 4; - const COPY_SIZE_BYTES: u8 = 3; - const COPY_ZERO_SIZE: usize = 0x10000; - + ) -> Result { let mut stream = Cursor::new(delta_obj.data_decompressed.as_slice()); - - // Read the base object size - // (Size Encoding) - let (base_size, result_size) = utils::read_delta_object_size(&mut stream).unwrap(); - - // Get the base object data - let base_info = &base_obj.data_decompressed; - assert_eq!(base_info.len(), base_size, "Base object size mismatch"); - - let mut result = Vec::with_capacity(result_size); - - loop { - // Check if the stream has ended, meaning the new object is done - let instruction = match utils::read_bytes(&mut stream) { - Ok([instruction]) => instruction, - Err(err) if err.kind() == ErrorKind::UnexpectedEof => break, - Err(err) => { - panic!( - "{}", - GitError::DeltaObjectError(format!("Wrong instruction in delta :{err}")) - ); - } - }; - - if instruction & COPY_INSTRUCTION_FLAG == 0 { - // Data instruction; the instruction byte specifies the number of data bytes - if instruction == 0 { - // Appending 0 bytes doesn't make sense, so git disallows it - panic!( - "{}", - GitError::DeltaObjectError(String::from("Invalid data instruction")) - ); - } - - let start = stream.position() as usize; - let end = start + instruction as usize; - let delta_data = *stream.get_ref(); - let data = delta_data.get(start..end).unwrap_or_else(|| { - panic!( - "{}", - GitError::DeltaObjectError("Invalid data instruction".to_string()) - ) - }); - result.extend_from_slice(data); - stream.set_position(end as u64); - } else { - // Copy instruction - // +----------+---------+---------+---------+---------+-------+-------+-------+ - // | 1xxxxxxx | offset1 | offset2 | offset3 | offset4 | size1 | size2 | size3 | - // +----------+---------+---------+---------+---------+-------+-------+-------+ - let mut nonzero_bytes = instruction; - let offset = - utils::read_partial_int(&mut stream, COPY_OFFSET_BYTES, &mut nonzero_bytes) - .unwrap(); - let mut size = - utils::read_partial_int(&mut stream, COPY_SIZE_BYTES, &mut nonzero_bytes) - .unwrap(); - if size == 0 { - // Copying 0 bytes doesn't make sense, so git assumes a different size - size = COPY_ZERO_SIZE; - } - // Copy bytes from the base object - let base_data = base_info.get(offset..(offset + size)).ok_or_else(|| { - GitError::DeltaObjectError("Invalid copy instruction".to_string()) - }); - - match base_data { - Ok(data) => result.extend_from_slice(data), - Err(e) => panic!("{}", e), - } - } - } - assert_eq!(result_size, result.len(), "Result size mismatch"); + let result = crate::delta::delta_decode(&mut stream, &base_obj.data_decompressed) + .map_err(|error| GitError::DeltaObjectError(error.to_string()))?; let hash = known_hash .unwrap_or_else(|| utils::calculate_object_hash(base_obj.object_type(), &result)); // create new obj from `delta_obj` & `result` instead of modifying `delta_obj` for heap-size recording - CacheObject { + Ok(CacheObject { info: CacheObjectInfo::BaseObject(base_obj.object_type(), hash), offset: delta_obj.offset, crc32: delta_obj.crc32, @@ -2471,7 +2416,7 @@ impl Pack { mem_recorder: None, is_delta_in_pack: delta_obj.is_delta_in_pack, known_hash: None, - } // Canonical form (Complete Object) + }) // Canonical form (Complete Object) // Memory recording will happen after this function returns. See `process_delta` } pub fn rebuild_zstdelta(delta_obj: CacheObject, base_obj: Arc) -> CacheObject { @@ -2816,6 +2761,7 @@ mod tests { callback: Some(callback), retention: Some(Arc::new(super::DecodeRetention::default())), skip_unneeded_objects: true, + error: Mutex::new(None), }); let obj = CacheObject { info: CacheObjectInfo::BaseObject(ObjectType::Blob, hash), @@ -3451,12 +3397,78 @@ mod tests { known_hash: None, }; - let rebuilt = Pack::rebuild_delta(delta, base); + let rebuilt = Pack::rebuild_delta(delta, base).unwrap(); assert_eq!(rebuilt.object_type(), ObjectType::Blob); assert_eq!(rebuilt.data_decompressed, b"hi there"); } + #[test] + fn test_rebuild_delta_malformed_stream_returns_error() { + let _guard = set_hash_kind_for_test(HashKind::Sha1); + let base = Arc::new(CacheObject::new_for_undeltified( + ObjectType::Blob, + b"hello".to_vec(), + 12, + 0, + )); + let delta = CacheObject { + info: CacheObjectInfo::OffsetDelta(12, 0), + offset: 20, + crc32: 0, + data_decompressed: Vec::new(), + mem_recorder: None, + is_delta_in_pack: true, + known_hash: None, + }; + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + Pack::rebuild_delta(delta, base) + })); + + assert!(result.is_ok(), "malformed delta should not panic"); + let result = result.unwrap(); + assert!( + matches!(result, Err(crate::errors::GitError::DeltaObjectError(_))), + "unexpected decode result: {result:?}" + ); + } + + #[test] + fn test_pack_decode_malformed_delta_returns_error_without_panic() { + let _guard = set_hash_kind_for_test(HashKind::Sha1); + let mut pack_data = Vec::new(); + pack_data.extend_from_slice(b"PACK"); + pack_data.extend_from_slice(&2u32.to_be_bytes()); + pack_data.extend_from_slice(&2u32.to_be_bytes()); + + let base_offset = pack_data.len(); + pack_data.push(0x31); + append_compressed(&mut pack_data, b"a"); + + let delta_offset = pack_data.len(); + let delta_payload = vec![0x01, 0x02, b'b']; + pack_data.push(0x63); + pack_data.push((delta_offset - base_offset) as u8); + append_compressed(&mut pack_data, &delta_payload); + + let trailer = Sha1::digest(&pack_data); + pack_data.extend_from_slice(&trailer); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let mut reader = Cursor::new(pack_data); + let mut pack = Pack::new(Some(1), None, None, true); + pack.decode(&mut reader, |_| {}, None::) + })); + + assert!(result.is_ok(), "malformed pack delta should not panic"); + let result = result.unwrap(); + assert!( + matches!(result, Err(crate::errors::GitError::DeltaObjectError(_))), + "unexpected decode result: {result:?}" + ); + } + #[test] #[cfg(target_pointer_width = "32")] fn test_pack_new_mem_limit_no_overflow_32bit() {