From 8f2fdc7aabdb1351ecaede19b462319f75ae3c64 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sun, 27 Jul 2025 20:07:03 +0300 Subject: [PATCH 1/9] perf: remove panics from TlvcReader --- tlvc/src/lib.rs | 96 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 68 insertions(+), 28 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 00b64d5..0b9cc70 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -50,8 +50,21 @@ impl ChunkHeader { /// Compute the length of this chunk in bytes, including the header, /// body, any padding, and the trailing checksum. + /// + /// ## Panics + /// + /// If the chunk's total length cannot be represented as a usize. pub fn total_len_in_bytes(&self) -> usize { - size_of::() + round_up_usize(self.len.get() as usize) + 4 + // Note: it would be nice to avoid the panic here, but it's not + // possible because the len field is public and can be mutated. + // Preferably the field(s) would be behind getters, in which case + // creation of ChunkHeader could check that the length field is + // reasonable. + const HEADER_AND_CHECKSUM_BYTES: usize = + size_of::() + size_of::(); + round_up_usize(self.len.get() as usize) + .and_then(|l| l.checked_add(HEADER_AND_CHECKSUM_BYTES)) + .unwrap() } } @@ -84,8 +97,12 @@ impl TlvcRead for &'_ [u8] { offset: u64, dest: &mut [u8], ) -> Result<(), TlvcReadError> { - let offset = usize::try_from(offset).unwrap(); - let end = offset.checked_add(dest.len()).unwrap(); + let Ok(offset) = usize::try_from(offset) else { + return Err(TlvcReadError::Truncated); + }; + let Some(end) = offset.checked_add(dest.len()) else { + return Err(TlvcReadError::Truncated); + }; dest.copy_from_slice(&self[offset..end]); Ok(()) } @@ -150,7 +167,7 @@ impl TlvcReader { /// Returns the number of bytes remaining in this reader. pub fn remaining(&self) -> u64 { - self.limit - self.position + self.limit.saturating_sub(self.position) } /// Destroys this reader and returns the original `source`, the byte @@ -188,9 +205,16 @@ impl TlvcReader { } let header = self.read_header()?; - let body_position = self.position + size_of::() as u64; - let body_len = round_up(u64::from(header.len.get())); - let chunk_end = body_position + body_len + 4; + // Note: this cannot wrap as read_header has checked it. + let body_position = + self.position.wrapping_add(size_of::() as u64); + // Note: this cannot wrap as adding 4 to a u32::MAX rounded up still + // fits nicely in a u64. + let body_and_checksum_len = + round_u32_up_to_u64(header.len.get()).wrapping_add(4); + let chunk_end = body_position + .checked_add(body_and_checksum_len) + .ok_or(TlvcReadError::Truncated)?; if chunk_end > self.limit { return Err(TlvcReadError::Truncated); @@ -245,13 +269,11 @@ impl TlvcReader { pub fn skip_chunk(&mut self) -> Result<(), TlvcReadError> { let h = self.read_header()?; - // Compute the overall size of the header, contents (rounded up for - // alignment), and the trailing checksum (which we're not going to - // check). - let size = size_of::() as u64 - + round_up(u64::from(h.len.get())) - + size_of::() as u64; - + // Compute the overall size of the contents (rounded up for alignment), + // header, and the trailing checksum (which we're not going to check). + // Note: we cannot wrap here as we go from a u32 to a u64. + let size = round_u32_up_to_u64(h.len.get()) + .wrapping_add((size_of::() + size_of::()) as u64); // Bump our new position forward as long as it doesn't cross our limit. // This may leave us zero-length. That's ok. let p = self @@ -347,7 +369,10 @@ impl ChunkHandle { TlvcReader { source: self.source.clone(), position: self.body_position, - limit: self.body_position + u64::from(self.header.len.get()), + limit: self + .body_position + .checked_add(u64::from(self.header.len.get())) + .unwrap(), } } @@ -363,22 +388,33 @@ impl ChunkHandle { where R: TlvcRead, { + // Caclulate the body checksum. let mut c = begin_body_crc(); - let end = self.body_position + self.header.len.get() as u64; + let end = self + .body_position + .checked_add(self.header.len.get() as u64) + .ok_or(TlvcReadError::Truncated)?; let mut pos = self.body_position; - while pos != end { - let portion = usize::try_from(end - pos) + while pos < end { + let portion = usize::try_from(end.wrapping_sub(pos)) .unwrap_or(usize::MAX) .min(buffer.len()); - self.source.read_exact(pos, &mut buffer[..portion])?; - c.update(&buffer[..portion]); - pos += u64::try_from(portion).unwrap(); + let buf = &mut buffer[..portion]; + self.source.read_exact(pos, buf)?; + c.update(buf); + // Note: this cannot wrap, as portion is necessarily less or equal + // to 'end - pos'; 'pos + >=(end - pos)' spells out '>=end', and + // end is above created from addition with pos without wrapping. + pos = pos.wrapping_add(u64::try_from(portion).unwrap()); } - let computed_checksum = c.finalize(); + + // Read the stored checksum at the end of the chunk and compare. let mut stored_checksum = 0u32; - self.source - .read_exact(round_up(end), stored_checksum.as_bytes_mut())?; + self.source.read_exact( + round_up(end).ok_or(TlvcReadError::Truncated)?, + stored_checksum.as_bytes_mut(), + )?; if computed_checksum != stored_checksum { Err(TlvcReadError::BodyCorrupt { @@ -407,12 +443,16 @@ pub fn compute_body_crc(data: &[u8]) -> u32 { c.finalize() } -fn round_up(x: u64) -> u64 { - (x + 0b11) & !0b11 +fn round_up(x: u64) -> Option { + Some(x.checked_add(0b11)? & !0b11) +} + +fn round_u32_up_to_u64(x: u32) -> u64 { + (x as u64).wrapping_add(0b11) & !0b11 } -fn round_up_usize(x: usize) -> usize { - (x + 0b11) & !0b11 +fn round_up_usize(x: usize) -> Option { + Some(x.checked_add(0b11)? & !0b11) } #[cfg(test)] From 0136b7e245942510c6cfa2b8402add1b986d4164 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sun, 27 Jul 2025 20:28:29 +0300 Subject: [PATCH 2/9] chore: checked_add for read_exact --- tlvc/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 0b9cc70..138dcb9 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -349,7 +349,11 @@ impl ChunkHandle { return Err(TlvcReadError::Truncated); } - self.source.read_exact(self.body_position + position, dest) + let Some(offset) = self.body_position.checked_add(position) else { + return Err(TlvcReadError::Truncated); + }; + + self.source.read_exact(offset, dest) } /// Produces a `TlvcReader` that can be used to interpret this chunk's body From ed8f2faf9f5570308c583725c2c9ee0ee756254f Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 00:14:05 +0300 Subject: [PATCH 3/9] fix: remove wrapping arithmetic usage where it is not the programmer's intention --- tlvc/src/lib.rs | 66 ++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 138dcb9..15dc68e 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -205,13 +205,16 @@ impl TlvcReader { } let header = self.read_header()?; - // Note: this cannot wrap as read_header has checked it. - let body_position = - self.position.wrapping_add(size_of::() as u64); + // SAFETY: read_header has performed checked_add on the same values and + // returned an Err(TlvcReadError::Truncated) if this did wrap. + let body_position = unsafe { + self.position.unchecked_add(size_of::() as u64) + }; // Note: this cannot wrap as adding 4 to a u32::MAX rounded up still - // fits nicely in a u64. - let body_and_checksum_len = - round_u32_up_to_u64(header.len.get()).wrapping_add(4); + // fits nicely in a u64. The compiler should remove this error branch. + let body_and_checksum_len = round_up_u32_to_u64(header.len.get()) + .checked_add(4) + .ok_or(TlvcReadError::Truncated)?; let chunk_end = body_position .checked_add(body_and_checksum_len) .ok_or(TlvcReadError::Truncated)?; @@ -271,9 +274,10 @@ impl TlvcReader { // Compute the overall size of the contents (rounded up for alignment), // header, and the trailing checksum (which we're not going to check). - // Note: we cannot wrap here as we go from a u32 to a u64. - let size = round_u32_up_to_u64(h.len.get()) - .wrapping_add((size_of::() + size_of::()) as u64); + // Note: we cannot wrap here as we go from a u32 to a u64. The compiler + // should remove the panic here. + let size = round_up_u32_to_u64(h.len.get()) + + (size_of::() + size_of::()) as u64; // Bump our new position forward as long as it doesn't cross our limit. // This may leave us zero-length. That's ok. let p = self @@ -394,29 +398,32 @@ impl ChunkHandle { { // Caclulate the body checksum. let mut c = begin_body_crc(); - let end = self - .body_position - .checked_add(self.header.len.get() as u64) + let mut pos = usize::try_from(self.body_position) + .ok() + .ok_or(TlvcReadError::Truncated)?; + let contents_len = usize::try_from(self.header.len.get()) + .ok() .ok_or(TlvcReadError::Truncated)?; - let mut pos = self.body_position; + let end = pos + .checked_add(contents_len) + .ok_or(TlvcReadError::Truncated)?; + let len = buffer.len(); while pos < end { - let portion = usize::try_from(end.wrapping_sub(pos)) - .unwrap_or(usize::MAX) - .min(buffer.len()); + let portion = (end - pos).min(len); let buf = &mut buffer[..portion]; - self.source.read_exact(pos, buf)?; + self.source.read_exact( + u64::try_from(pos).ok().ok_or(TlvcReadError::Truncated)?, + buf, + )?; c.update(buf); - // Note: this cannot wrap, as portion is necessarily less or equal - // to 'end - pos'; 'pos + >=(end - pos)' spells out '>=end', and - // end is above created from addition with pos without wrapping. - pos = pos.wrapping_add(u64::try_from(portion).unwrap()); + pos += portion; } let computed_checksum = c.finalize(); // Read the stored checksum at the end of the chunk and compare. let mut stored_checksum = 0u32; self.source.read_exact( - round_up(end).ok_or(TlvcReadError::Truncated)?, + round_up_usize_to_u64(end).ok_or(TlvcReadError::Truncated)?, stored_checksum.as_bytes_mut(), )?; @@ -447,18 +454,21 @@ pub fn compute_body_crc(data: &[u8]) -> u32 { c.finalize() } -fn round_up(x: u64) -> Option { - Some(x.checked_add(0b11)? & !0b11) -} - -fn round_u32_up_to_u64(x: u32) -> u64 { - (x as u64).wrapping_add(0b11) & !0b11 +#[inline(always)] +fn round_up_u32_to_u64(x: u32) -> u64 { + (x as u64 + 0b11) & !0b11 } +#[inline(always)] fn round_up_usize(x: usize) -> Option { Some(x.checked_add(0b11)? & !0b11) } +#[inline(always)] +fn round_up_usize_to_u64(x: usize) -> Option { + Some((u64::try_from(x).ok()?).checked_add(0b11)? & !0b11) +} + #[cfg(test)] mod tests { use super::*; From b06ae61d0ca7edc16916f0c6f8898b96e8165c89 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 00:25:39 +0300 Subject: [PATCH 4/9] nit --- tlvc/src/lib.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 15dc68e..1b8b172 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -206,15 +206,13 @@ impl TlvcReader { let header = self.read_header()?; // SAFETY: read_header has performed checked_add on the same values and - // returned an Err(TlvcReadError::Truncated) if this did wrap. + // returned an Err(TlvcReadError::Truncated) if this did overflow. let body_position = unsafe { self.position.unchecked_add(size_of::() as u64) }; - // Note: this cannot wrap as adding 4 to a u32::MAX rounded up still - // fits nicely in a u64. The compiler should remove this error branch. - let body_and_checksum_len = round_up_u32_to_u64(header.len.get()) - .checked_add(4) - .ok_or(TlvcReadError::Truncated)?; + // Note: this cannot overflow as we go from a u32 to a u64, and the + // compiler sees it too and removes the panic branch here. + let body_and_checksum_len = round_up_u32_to_u64(header.len.get()) + 4; let chunk_end = body_position .checked_add(body_and_checksum_len) .ok_or(TlvcReadError::Truncated)?; @@ -274,8 +272,8 @@ impl TlvcReader { // Compute the overall size of the contents (rounded up for alignment), // header, and the trailing checksum (which we're not going to check). - // Note: we cannot wrap here as we go from a u32 to a u64. The compiler - // should remove the panic here. + // Note: this cannot overflow as we go from a u32 to a u64, and the + // compiler sees it too and removes the panic branch here. let size = round_up_u32_to_u64(h.len.get()) + (size_of::() + size_of::()) as u64; // Bump our new position forward as long as it doesn't cross our limit. From f7e9d2c98b7d30969c0ee27ef01d26f873e30d24 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 00:57:47 +0300 Subject: [PATCH 5/9] more nit --- tlvc/src/lib.rs | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 1b8b172..8c672d6 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -89,7 +89,7 @@ pub trait TlvcRead: Clone { impl TlvcRead for &'_ [u8] { type Error = core::convert::Infallible; fn extent(&self) -> Result> { - Ok(u64::try_from(self.len()).unwrap()) + u64::try_from(self.len()).map_err(|_| TlvcReadError::Truncated) } fn read_exact( @@ -97,12 +97,11 @@ impl TlvcRead for &'_ [u8] { offset: u64, dest: &mut [u8], ) -> Result<(), TlvcReadError> { - let Ok(offset) = usize::try_from(offset) else { - return Err(TlvcReadError::Truncated); - }; - let Some(end) = offset.checked_add(dest.len()) else { - return Err(TlvcReadError::Truncated); - }; + let offset = + usize::try_from(offset).map_err(|_| TlvcReadError::Truncated)?; + let end = offset + .checked_add(dest.len()) + .ok_or(TlvcReadError::Truncated)?; dest.copy_from_slice(&self[offset..end]); Ok(()) } @@ -212,9 +211,9 @@ impl TlvcReader { }; // Note: this cannot overflow as we go from a u32 to a u64, and the // compiler sees it too and removes the panic branch here. - let body_and_checksum_len = round_up_u32_to_u64(header.len.get()) + 4; + let body_and_checksum_size = round_up_u32_to_u64(header.len.get()) + 4; let chunk_end = body_position - .checked_add(body_and_checksum_len) + .checked_add(body_and_checksum_size) .ok_or(TlvcReadError::Truncated)?; if chunk_end > self.limit { @@ -274,20 +273,20 @@ impl TlvcReader { // header, and the trailing checksum (which we're not going to check). // Note: this cannot overflow as we go from a u32 to a u64, and the // compiler sees it too and removes the panic branch here. - let size = round_up_u32_to_u64(h.len.get()) + let chunk_size = round_up_u32_to_u64(h.len.get()) + (size_of::() + size_of::()) as u64; // Bump our new position forward as long as it doesn't cross our limit. // This may leave us zero-length. That's ok. - let p = self + let chunk_end = self .position - .checked_add(size) + .checked_add(chunk_size) .ok_or(TlvcReadError::Truncated)?; - if p > self.limit { + if chunk_end > self.limit { return Err(TlvcReadError::Truncated); } - self.position = p; + self.position = chunk_end; Ok(()) } @@ -345,15 +344,20 @@ impl ChunkHandle { R: TlvcRead, { let end = position - .checked_add(u64::try_from(dest.len()).unwrap()) + .checked_add( + u64::try_from(dest.len()) + .ok() + .ok_or(TlvcReadError::Truncated)?, + ) .ok_or(TlvcReadError::Truncated)?; if end > self.len() { return Err(TlvcReadError::Truncated); } - let Some(offset) = self.body_position.checked_add(position) else { - return Err(TlvcReadError::Truncated); - }; + let offset = self + .body_position + .checked_add(position) + .ok_or(TlvcReadError::Truncated)?; self.source.read_exact(offset, dest) } From aef1aab9e375871a314ee4c1f59fd6bee753ec72 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 00:58:26 +0300 Subject: [PATCH 6/9] nit: unchecked_add in read_as_chunks to avoid one more panic --- tlvc/src/lib.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 8c672d6..42e0308 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -212,6 +212,9 @@ impl TlvcReader { // Note: this cannot overflow as we go from a u32 to a u64, and the // compiler sees it too and removes the panic branch here. let body_and_checksum_size = round_up_u32_to_u64(header.len.get()) + 4; + // Note: ChunkHandle::read_as_chunks assumes that this check is + // performed. Removing this would make the SAFETY comment there invalid + // and risk undefined behaviour. let chunk_end = body_position .checked_add(body_and_checksum_size) .ok_or(TlvcReadError::Truncated)?; @@ -221,6 +224,9 @@ impl TlvcReader { } self.position = chunk_end; + // Note: ChunkHandle::read_as_chunks assumes that this is the only + // code path creating handles, and that above + // body_position + header.len is checked to not overflow. Ok(Some(ChunkHandle { source: self.source.clone(), header, @@ -379,10 +385,14 @@ impl ChunkHandle { TlvcReader { source: self.source.clone(), position: self.body_position, - limit: self - .body_position - .checked_add(u64::from(self.header.len.get())) - .unwrap(), + // SAFETY: creation of ChunkHandle in TlvcReader::next checks that + // body_position + header.len does not overflow. ChunkHandle has no + // '&mut self' methods and the fields are private, so this addition + // still cannot overflow. + limit: unsafe { + self.body_position + .unchecked_add(u64::from(self.header.len.get())) + }, } } From 28c819d37c34074c43e61936fa339bdcc2daccbb Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 01:24:53 +0300 Subject: [PATCH 7/9] Update tlvc/src/lib.rs Co-authored-by: Eliza Weisman --- tlvc/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 42e0308..a6a36f4 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -352,8 +352,7 @@ impl ChunkHandle { let end = position .checked_add( u64::try_from(dest.len()) - .ok() - .ok_or(TlvcReadError::Truncated)?, + .map_err(|_| TlvcReadError::Truncated)?, ) .ok_or(TlvcReadError::Truncated)?; if end > self.len() { From 8d37061fae0de708f4620d95a7fc6832188fc27c Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 01:25:10 +0300 Subject: [PATCH 8/9] Update tlvc/src/lib.rs Co-authored-by: Eliza Weisman --- tlvc/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index a6a36f4..8579f4d 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -423,7 +423,7 @@ impl ChunkHandle { let portion = (end - pos).min(len); let buf = &mut buffer[..portion]; self.source.read_exact( - u64::try_from(pos).ok().ok_or(TlvcReadError::Truncated)?, + u64::try_from(pos).map_err(|_| TlvcReadError::Truncated)?, buf, )?; c.update(buf); From 77c8ea377cf75b64e6be76e5d6db3fa901fedb14 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Wed, 30 Jul 2025 01:27:04 +0300 Subject: [PATCH 9/9] fix: other ok().ok_or() oddness --- tlvc/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tlvc/src/lib.rs b/tlvc/src/lib.rs index 8579f4d..bc80386 100644 --- a/tlvc/src/lib.rs +++ b/tlvc/src/lib.rs @@ -410,11 +410,9 @@ impl ChunkHandle { // Caclulate the body checksum. let mut c = begin_body_crc(); let mut pos = usize::try_from(self.body_position) - .ok() - .ok_or(TlvcReadError::Truncated)?; + .map_err(|_| TlvcReadError::Truncated)?; let contents_len = usize::try_from(self.header.len.get()) - .ok() - .ok_or(TlvcReadError::Truncated)?; + .map_err(|_| TlvcReadError::Truncated)?; let end = pos .checked_add(contents_len) .ok_or(TlvcReadError::Truncated)?;