Undefined behavior during decoding #75
Comments
|
I was able to reproduce this by running tests with miri ( |
|
EDIT: the first point ends up being wrong, but fixing it allowed miri to continue and expose the real problem in point 2. Here's what I found for now:
diff --git a/src/decoding/ringbuffer.rs b/src/decoding/ringbuffer.rs
index e364d90..92f2987 100644
--- a/src/decoding/ringbuffer.rs
+++ b/src/decoding/ringbuffer.rs
@@ -1,5 +1,5 @@
use alloc::alloc::{alloc, dealloc};
-use core::{alloc::Layout, ptr::NonNull, slice};
+use core::{alloc::Layout, mem::MaybeUninit, ptr::NonNull, slice};
pub struct RingBuffer {
// Safety invariants:
@@ -295,6 +295,18 @@ impl RingBuffer {
/// 2. More then len reserved space so we do not write out-of-bounds
#[warn(unsafe_op_in_unsafe_fn)]
pub unsafe fn extend_from_within_unchecked(&mut self, start: usize, len: usize) {
+ std::println!("-- extend_from_within_unchecked --");
+ std::println!("buf ptr: {}..{}", self.buf.as_ptr() as usize, unsafe {
+ self.buf.as_ptr().add(self.cap)
+ }
+ as usize);
+ std::println!("len: {}", self.len());
+ std::println!("capacity: {}", self.cap);
+ std::println!("head: {}", self.head);
+ std::println!("tail: {}", self.tail);
+ std::println!("copy start: {}", start);
+ std::println!("copy len: {}", len);
+
if self.head < self.tail {
// continous data slice |____HDDDDDDDT_____|
let after_tail = usize::min(len, self.cap - self.tail);
@@ -307,7 +319,22 @@ impl RingBuffer {
copy_bytes_overshooting(src, dst, after_tail);
if after_tail < len {
- let src = (src.0.add(after_tail), src.1 - after_tail);
+ let src = (
+ src.0.add(after_tail),
+ if (src.1 - after_tail) == 93 {
+ // The next issue that comes up is that one of the calls
+ // to `copy_bytes_overshooting` gives a src len of 93,
+ // which is incorrect because we have just 10 bytes
+ // before the end of the allocation. Giving it a correct
+ // value makes it happy.
+ // Maybe it's ok to give it 93 but then something is wrong
+ // when handling the wrapping around?
+ std::println!("INCORRECTLY CALCULATED");
+ 10
+ } else {
+ src.1 - after_tail
+ },
+ );
let dst = (self.buf.as_ptr(), self.head);
copy_bytes_overshooting(src, dst, len - after_tail);
}
@@ -455,11 +482,23 @@ unsafe fn copy_bytes_overshooting(
const COPY_AT_ONCE_SIZE: usize = core::mem::size_of::<CopyType>();
let min_buffer_size = usize::min(src.1, dst.1);
+ std::println!("src ptr: {}", src.0 as usize);
+ std::println!("src len: {}", src.1);
+ std::println!("dst ptr: {}", dst.0 as usize);
+ std::println!("dst len: {}", dst.1);
+
// Can copy in just one read+write, very common case
if min_buffer_size >= COPY_AT_ONCE_SIZE && copy_at_least <= COPY_AT_ONCE_SIZE {
+ /*
dst.0
.cast::<CopyType>()
- .write_unaligned(src.0.cast::<CopyType>().read_unaligned())
+ .write_unaligned(src.0.cast::<CopyType>().read_unaligned());
+ */
+
+ // We need `MaybeUninit` because part of the memory we are copying may be uninitialized
+ dst.0
+ .cast::<MaybeUninit<CopyType>>()
+ .write_unaligned(src.0.cast::<MaybeUninit<CopyType>>().read_unaligned());
} else {
let copy_multiple = copy_at_least.next_multiple_of(COPY_AT_ONCE_SIZE);
// Can copy in multiple simple instructions |
|
I think I get what's happening. This diff --git a/src/decoding/ringbuffer.rs b/src/decoding/ringbuffer.rs
index e364d90..573cbea 100644
--- a/src/decoding/ringbuffer.rs
+++ b/src/decoding/ringbuffer.rs
@@ -309,7 +309,11 @@ impl RingBuffer {
if after_tail < len {
let src = (src.0.add(after_tail), src.1 - after_tail);
let dst = (self.buf.as_ptr(), self.head);
- copy_bytes_overshooting(src, dst, len - after_tail);
+ let copy_at_least = len - after_tail;
+ debug_assert!(
+ (self.head + start) + after_tail + src.1 + copy_at_least <= self.cap
+ );
+ copy_bytes_overshooting(src, dst, copy_at_least);
}
}
} else { |
|
Could anyone help me test #76? |
|
Meanwhile I've opened a draft for the RustSec advisory at rustsec/advisory-db#2147 |
|
I tried to reproduce this on latest master (without the PR with the fix) under Memory Sanitizer, which should catch any reads from uninitialized memory. However, I was not able to trigger this issue. I wonder if this is a limitation of the fuzzing harness? Could someone try this code under miri and let me know if the issue is observable under miri instead of MSAN? zstd-rs/fuzz/fuzz_targets/decode.rs Lines 8 to 15 in 78c4172 |
|
I'm a noob at this, but tests were able to catch the issue both with Memory Sanitizer and Address Sanitizer. I'm not sure how to run fuzzing with miri, but given enough iterations the default config may catch it on |
|
There's no way to run fuzzing with miri. |
|
Sorry for chiming in so late, that's definitely something to wake up to. I think @paolobarbolini has found and fixed the issue in #76 (Thanks!!) I'll look into including miri in the CI to hopefully catch something like this earlier in the future. |
|
Update:
|
|
@Shnatsel I wanted to share my thoughts on your input regarding the sanitizers and fuzzing
Actually hitting this unintentionally is pretty unlikely. This only happens when
Making the fuzzer generate valid sequences isn't easy, the format is pretty unforgiving in that regard. So the fuzzer doesn't really run this code a lot. In combination this explains why the fuzzer doesn't easily hit this UB. The It could be worthwile to fuzz the ringbuffer/sequence-execution separately by generating random sequences though. |
|
@KillingSpark thanks, those steps all seem correct to me! If miri proves to be too slow, tests with Address Sanitizer should cover most of the same issues but run much faster. Regarding fuzzing: I've changed the fuzzing harness to use a |
|
This issue has been assigned RUSTSEC-2024-0400. The Github Security Advisory system should pick it up in a few hours. |
|
ASan cannot detect uninitialized memory read/writes sadly. One alternative to MSan is using valgrind (with memcheck), but it's also pretty slow, but is easier to use than MSan (and miri if non-rust code is used -- not interesting for this crate). For my use case valgrind is faster than miri (But I'd still recommend Miri, since Rust defines things like two mutable refs to the same address as UB, which can easily happen when dereferencing pointers) |
Tried to use this crate as an alternative to the zstd crate, to allow tests running with miri, but it crashed due to UB:
The invoking code is not really special:
where
offsetmight be an arbitrary offset within a bigger bufferThe text was updated successfully, but these errors were encountered: