BurntSushi / rust-snappy

Snappy compression implemented in Rust (including the Snappy frame format).
BSD 3-Clause "New" or "Revised" License
444 stars 43 forks source link

Undefined behavior in `Decompress::read_copy` #56

Closed icmccorm closed 9 months ago

icmccorm commented 9 months ago

I've found an instance of undefined behavior under the Tree Borrows model. This occurs in the implementation of Decompress::read_copy. This occurs within a loop on lines 299-312 in ./src/decompress.rs.

let mut dstp = self.dst.as_mut_ptr().add(self.d);           
let mut srcp = dstp.sub(offset);
loop {
    debug_assert!(dstp >= srcp);
    let diff = (dstp as usize) - (srcp as usize);
    if diff >= 16 {
        break;
    }
    // srcp and dstp can overlap, so use ptr::copy.
    debug_assert!(self.d + 16 <= self.dst.len());
    ptr::copy(srcp, dstp, 16);
    self.d += diff as usize;
    dstp = dstp.add(diff);
}

Both dstp and srcp are derived from the field self.dst and share the same access tags. They're considered Reserved under tree borrows until the first call to ptr::copy, after which they transition to Active.

The field self.dst is also reborrowed immutably on each iteration. This occurs within the second debug assertion as a result of the expression self.dst.len(). From the perspective of the dstp and srcp pointers, the expression self.dst.len() reborrows against self.dst, counting as a foreign read access. This is totally fine during the first iteration of the loop, as Reserved tolerates foreign reads. However, Active does not allow foreign reads, so both srcp and destp will become invalid to use during the second iteration, making ptr::copy undefined behavior.

This can be fixed by saving the value of self.dst.len() in a variable outside the loop and then referencing this value during iteration.