Skip to content

Commit

Permalink
Fixes an overflow bug.
Browse files Browse the repository at this point in the history
On a 32 bit system, given the right values for a literal length, it was
possible to overflow the bounds check and cause an unsafe use of
ptr::copy_overlapping. We avoid the unsafety by moving more of the
calculations to 64 bits.

Fixes #3.
  • Loading branch information
BurntSushi committed Aug 16, 2016
1 parent 99e7b31 commit b5e822d
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 11 deletions.
25 changes: 14 additions & 11 deletions src/decompress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,10 @@ impl<'s, 'd> Decompress<'s, 'd> {
#[inline(always)]
fn read_literal(
&mut self,
mut len: usize,
len: usize,
) -> Result<()> {
debug_assert!(len <= 64);
let mut len = len as u64;
// As an optimization for the common case, if the literal length is
// <=16 and we have enough room in both `src` and `dst`, copy the
// literal using unaligned loads and stores.
Expand All @@ -186,8 +187,8 @@ impl<'s, 'd> Decompress<'s, 'd> {
// Hopefully uses SIMD registers for 128 bit load/store.
ptr::copy_nonoverlapping(srcp, dstp, 16);
}
self.d += len;
self.s += len;
self.d += len as usize;
self.s += len as usize;
return Ok(());
}
// When the length is bigger than 60, it indicates that we need to read
Expand All @@ -204,16 +205,18 @@ impl<'s, 'd> Decompress<'s, 'd> {
}
// Since we know there are 4 bytes left to read, read a 32 bit LE
// integer and mask away the bits we don't need.
let byte_count = len - 60;
len = LE::read_u32(&self.src[self.s..]) as usize;
len = (len & WORD_MASK[byte_count]) + 1;
let byte_count = len as usize - 60;
len = LE::read_u32(&self.src[self.s..]) as u64;
len = (len & (WORD_MASK[byte_count] as u64)) + 1;
self.s += byte_count;
}
// If there's not enough buffer left to load or store this literal,
// then the input is corrupt.
if self.s + len > self.src.len() || self.d + len > self.dst.len() {
// if self.s + len > self.src.len() || self.d + len > self.dst.len() {
if ((self.src.len() - self.s) as u64) < len
|| ((self.dst.len() - self.d) as u64) < len {
return Err(Error::Literal {
len: len as u64,
len: len,
src_len: (self.src.len() - self.s) as u64,
dst_len: (self.dst.len() - self.d) as u64,
});
Expand All @@ -223,10 +226,10 @@ impl<'s, 'd> Decompress<'s, 'd> {
// is correct.
let srcp = self.src.as_ptr().offset(self.s as isize);
let dstp = self.dst.as_mut_ptr().offset(self.d as isize);
ptr::copy_nonoverlapping(srcp, dstp, len);
ptr::copy_nonoverlapping(srcp, dstp, len as usize);
}
self.s += len;
self.d += len;
self.s += len as usize;
self.d += len as usize;
Ok(())
}

Expand Down
18 changes: 18 additions & 0 deletions src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,24 @@ fn qc_cmpcpp() {
.quickcheck(p as fn(_) -> _);
}

// Regression tests.

// See: https://github.com/BurntSushi/rust-snappy/issues/3
#[cfg(target_pointer_width = "32")]
testerrored!(err_lit_len_overflow1, &b"\x11\x00\x00\xfc\xfe\xff\xff\xff"[..],
Error::Literal {
len: ::std::u32::MAX as u64,
src_len: 0,
dst_len: 16,
});
#[cfg(target_pointer_width = "32")]
testerrored!(err_lit_len_overflow2, &b"\x11\x00\x00\xfc\xff\xff\xff\xff"[..],
Error::Literal {
len: ::std::u32::MAX as u64 + 1,
src_len: 0,
dst_len: 16,
});

// Helper functions.

fn press(bytes: &[u8]) -> Vec<u8> {
Expand Down

0 comments on commit b5e822d

Please sign in to comment.