From 8e3ce78616dbf9bc16b8226a0be60207582d4a05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D1=80=D1=82=D1=91=D0=BC=20=D0=9F=D0=B0=D0=B2=D0=BB?= =?UTF-8?q?=D0=BE=D0=B2=20=5BArtyom=20Pavlov=5D?= Date: Thu, 11 Jun 2026 18:19:39 +0300 Subject: [PATCH 1/2] block-buffer: fix exception safety --- block-buffer/CHANGELOG.md | 6 +++ block-buffer/src/lib.rs | 57 +++++++++++++++++------- block-buffer/src/read.rs | 21 ++++++++- block-buffer/tests/mod.rs | 93 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 158 insertions(+), 19 deletions(-) diff --git a/block-buffer/CHANGELOG.md b/block-buffer/CHANGELOG.md index fc5ff8c4..7315efce 100644 --- a/block-buffer/CHANGELOG.md +++ b/block-buffer/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## 0.12.1 (UNRELEASED) +### Fixed +- Exception safety bug ([#1487]) + +[#1487]: https://github.com/RustCrypto/utils/pull/1487 + ## 0.12.0 (2026-02-24) ### Added - `BlockSizes` trait implemented for sizes bigger than `U0` and smaller than `U256` ([#1455]) diff --git a/block-buffer/src/lib.rs b/block-buffer/src/lib.rs index 6f8ac67c..827d68db 100644 --- a/block-buffer/src/lib.rs +++ b/block-buffer/src/lib.rs @@ -184,12 +184,18 @@ impl BlockBuffer { if pos != 0 { let (left, right) = input.split_at(rem); input = right; + + let g = ResetGuard(self); + let buf = &mut g.0.buffer; // SAFETY: length of `left` is equal to number of remaining bytes in `buffer`, // so we can copy data into it and process `buffer` as fully initialized block. + // Note that this code can temporarily break the eager buffer invariant, + // but we reset the buffer immediately after `compress` using `Drop` impl of + // `ResetGuard`, so this code is safe even if `compress` panics. let block = unsafe { - let buf_ptr = self.buffer.as_mut_ptr().cast::().add(pos); + let buf_ptr = buf.as_mut_ptr().cast::().add(pos); ptr::copy_nonoverlapping(left.as_ptr(), buf_ptr, left.len()); - self.buffer.assume_init_ref() + buf.assume_init_ref() }; compress(slice::from_ref(block)); } @@ -363,22 +369,34 @@ impl BlockBuffer { suffix: &[u8], mut compress: impl FnMut(&Array), ) { - assert!(suffix.len() <= BS::USIZE, "suffix is too long"); let pos = self.get_pos(); - let mut buf = self.pad_with_zeros(); - buf[pos] = delim; - - let n = self.size() - suffix.len(); - if self.size() - pos - 1 < suffix.len() { - compress(&buf); + let size = self.size(); + // Number of bytes remaining in the buffer after `delim` is written to it + // This never underflows since for eager buffers `size` is always greater than `pos`. + let pad_len = size - pos - 1; + + let suffix_dst_pos = size + .checked_sub(suffix.len()) + .expect("suffix must be smaller than buffer block size"); + + let g = ResetGuard(self); + // SAFETY: we fully initialize the buffer. Note that we may temporarily break + // the buffer invariant, but we restore it using `ResetGuard`, + // which works even if `compress` panics. + let buf = unsafe { + let p: *mut u8 = g.0.buffer.as_mut_ptr().cast::().add(pos); + ptr::write(p, delim); + ptr::write_bytes(p.add(1), 0, pad_len); + g.0.buffer.assume_init_mut() + }; + + if pad_len < suffix.len() { + compress(buf); buf.fill(0); - buf[n..].copy_from_slice(suffix); - compress(&buf); - } else { - buf[n..].copy_from_slice(suffix); - compress(&buf); } - self.reset(); + + buf[suffix_dst_pos..].copy_from_slice(suffix); + compress(buf); } /// Pad message with 0x80, zeros and 64-bit message length using @@ -422,3 +440,12 @@ impl Drop for BlockBuffer { #[cfg(feature = "zeroize")] impl ZeroizeOnDrop for BlockBuffer {} + +/// Resets the referenced buffer on drop. +struct ResetGuard<'a, BS: BlockSizes, K: BufferKind>(&'a mut BlockBuffer); + +impl Drop for ResetGuard<'_, BS, K> { + fn drop(&mut self) { + self.0.reset(); + } +} diff --git a/block-buffer/src/read.rs b/block-buffer/src/read.rs index f72aabd6..1bd016e2 100644 --- a/block-buffer/src/read.rs +++ b/block-buffer/src/read.rs @@ -110,8 +110,16 @@ impl ReadBuffer { } assert!(read_len < BS::USIZE); - gen_block(&mut self.buffer); - read_fn(&self.buffer[..read_len]); + let g = ResetGuard(self); + let buf = &mut g.0.buffer; + + // Note that generated block is likely to break the `ReadBuffer` invariant. + // We restore it using `set_pos_unchecked` below and in case if one of the closures + // panic the buffer gets reset by the guard. + gen_block(buf); + read_fn(&buf[..read_len]); + + core::mem::forget(g); // We checked that `read_len` satisfies the `set_pos_unchecked` safety contract unsafe { self.set_pos_unchecked(read_len) }; @@ -180,3 +188,12 @@ impl Drop for ReadBuffer { #[cfg(feature = "zeroize")] impl zeroize::ZeroizeOnDrop for ReadBuffer {} + +/// Resets the referenced buffer on drop. +struct ResetGuard<'a, BS: BlockSizes>(&'a mut ReadBuffer); + +impl Drop for ResetGuard<'_, BS> { + fn drop(&mut self) { + self.0.reset(); + } +} diff --git a/block-buffer/tests/mod.rs b/block-buffer/tests/mod.rs index 9a4e5ca5..0958999e 100644 --- a/block-buffer/tests/mod.rs +++ b/block-buffer/tests/mod.rs @@ -9,8 +9,11 @@ use block_buffer::{ }; use hex_literal::hex; +use core::{array, panic::AssertUnwindSafe}; +use std::panic::catch_unwind; + #[test] -fn test_eager_digest_pad() { +fn test_eager_digest() { let mut buf = EagerBuffer::::default(); let inputs = [ &b"01234567"[..], @@ -45,7 +48,7 @@ fn test_eager_digest_pad() { } #[test] -fn test_lazy_digest_pad() { +fn test_lazy_digest() { let mut buf = LazyBuffer::::default(); let inputs = [ &b"01234567"[..], @@ -78,6 +81,38 @@ fn test_lazy_digest_pad() { assert_eq!(buf.get_pos(), 0); } +#[test] +fn digest_pad_combinations() { + let delim = 0x80; + let data: [u8; 7] = array::from_fn(|i| u8::try_from(i).unwrap()); + let suffix: [u8; 7] = array::from_fn(|i| u8::try_from(i + 0x10).unwrap()); + + for data_len in 0..data.len() { + for suffix_len in 0..suffix.len() { + let data = &data[..data_len]; + let suffix = &suffix[..suffix_len]; + let mut buf = EagerBuffer::::default(); + + buf.digest_blocks(data, |_| panic!("should not be called")); + + let mut accum = Vec::with_capacity(2 * buf.size()); + buf.digest_pad(delim, suffix, |block| accum.extend_from_slice(block)); + + assert!(accum.len() <= 2 * buf.size()); + assert_eq!(buf.get_pos(), 0); + + let (res_data, rem) = accum.split_at(data_len); + let (res_delim, rem) = rem.split_at(1); + let (res_zeros, res_suffix) = rem.split_at(rem.len() - suffix_len); + + assert_eq!(res_data, data); + assert_eq!(res_delim, &[delim]); + assert!(res_zeros.iter().all(|&b| b == 0)); + assert_eq!(res_suffix, suffix); + } + } +} + #[test] fn test_read() { type Buf = ReadBuffer; @@ -344,3 +379,57 @@ fn test_read_serialize() { let buf = Array([4, 0, 0, 1]); assert!(Buf::deserialize(&buf).is_err()); } + +#[test] +fn eager_buffer_exception_safety() { + let mut buf = EagerBuffer::::default(); + + let res = catch_unwind(AssertUnwindSafe(|| { + buf.digest_blocks(b"ab", |_| {}); + buf.digest_blocks(b"cd", |_| panic!("compression panic")); + })); + + assert!(res.is_err()); + let _ = buf.get_pos(); + + let mut buf = EagerBuffer::::default(); + + let res = catch_unwind(AssertUnwindSafe(|| { + buf.digest_pad(0x80, &[0xFF; 2], |_| panic!("compression panic")); + })); + + assert!(res.is_err()); + let _ = buf.get_pos(); +} + +#[test] +fn read_buffer_exception_safety() { + let mut buf = ReadBuffer::::default(); + + let res = catch_unwind(AssertUnwindSafe(|| { + buf.write_block( + 1, + |block| { + block[0] = 0xFF; + panic!("block generation panic"); + }, + |_| {}, + ); + })); + + assert!(res.is_err()); + let _ = buf.get_pos(); + + let mut buf = ReadBuffer::::default(); + + let res = catch_unwind(AssertUnwindSafe(|| { + buf.write_block( + 1, + |block| block.0 = [0xFF; 4], + |_| panic!("data read panic"), + ); + })); + + assert!(res.is_err()); + let _ = buf.get_pos(); +} From 5b2667634d65acbd427448cc16ef018a18b10af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D1=80=D1=82=D1=91=D0=BC=20=D0=9F=D0=B0=D0=B2=D0=BB?= =?UTF-8?q?=D0=BE=D0=B2=20=5BArtyom=20Pavlov=5D?= Date: Thu, 11 Jun 2026 18:22:29 +0300 Subject: [PATCH 2/2] Add miri job --- .github/workflows/block-buffer.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/workflows/block-buffer.yml b/.github/workflows/block-buffer.yml index ca14c29a..3e04d206 100644 --- a/.github/workflows/block-buffer.yml +++ b/.github/workflows/block-buffer.yml @@ -64,3 +64,19 @@ jobs: toolchain: ${{ matrix.rust }} - run: cargo test - run: cargo test --all-features + + miri: + runs-on: ubuntu-latest + strategy: + matrix: + target: + - x86_64-unknown-linux-gnu + - s390x-unknown-linux-gnu + steps: + - uses: actions/checkout@v6 + - uses: RustCrypto/actions/cargo-cache@master + - uses: dtolnay/rust-toolchain@master + with: + toolchain: nightly + components: miri + - run: cargo miri test --all-features --target ${{ matrix.target }}