Unsound usages of VecFromRawParts

[llooFlashooll]

Hi, I am scanning the abomination in the latest version with my own static analyzer tool.

Unsafe conversion found at: src/lib.rs#L496

#[inline]
unsafe fn exhume<'a,'b>(&'a mut self, bytes: &'b mut [u8]) -> Option<&'b mut [u8]> {

   // extract memory from bytes to back our vector
   let binary_len = self.len() * mem::size_of::<T>();
   if binary_len > bytes.len() { None }
   else {
      let (mine, mut rest) = bytes.split_at_mut(binary_len);
      let slice = std::slice::from_raw_parts_mut(mine.as_mut_ptr() as *mut T, self.len());
      std::ptr::write(self, Vec::from_raw_parts(slice.as_mut_ptr(), self.len(), self.len()));
      for element in self.iter_mut() {
            let temp = rest;             // temp variable explains lifetimes (mysterious!)
            rest = element.exhume(temp)?;
      }
      Some(rest)
   }
}

This unsound implementation of Vec::from_raw_parts would create a dangling pointer issues if the mine is dropped automatically before the rest is used. The 'mem::forget' function can be used to avoid the issue.

This would potentially cause undefined behaviors in Rust. If we further manipulate the problematic converted types, it would potentially lead to different consequences such as uaf or double free. I am reporting this issue for your attention.

[frankmcsherry]

Thanks for the heads up! I apologize, but I can't actually tell what you are saying. It might be different words meaning different things to different folks, so let me ask!

mine is a &mut [u8]; I don't understand what is meant by "if [it] is dropped automatically". I'm not aware of drop behavior for &mut references doing anything.

Also, no idea what "autograd" is. Is it relevant, or a copy/paste-o?

That being said, 100% there's all sorts of undefined behavior here, not least because I can't find the definition of what behavior is defined for Rust in unsafe blocks. But I think they reserve "unsound" for uses of unsafe that expose safe code to undefined behavior, and I don't think this does that. For sure lots of "unspecified invariants that must be upheld", to the point that I recommend not using the code at all!

[llooFlashooll]

Hi, thanks for your kind and detailed response! I am just up to work due to the jet lag. Sorry for the "autograd" mis-typed. I reported several repos and this is a mistake.

Since mine is a [u8] (I think mine.as_mut_ptr crates a &mut [u8]), how does the lifetime of this mine be calculated? I am wondering whether it would be automatically dropped and become dangling after the end of the function scope.

[frankmcsherry]

No worries!

In this context, mine is a &mut [u8] that derives from the bytes: &mut [u8] argument to the function, and whose lifetime should match that of bytes. Neither of these should result in drop logic. Perhaps what you (or your tools) are getting at is that the lifetimes that come in likely want to be related e.g. potentially through 'b : 'a: the typed reference shouldn't outlive the bytes reference.

Without wanting to pretend that there aren't problems, the contexts in which exhume is called, specifically the decode method, "ensure" that the lifetime of the typed reference is the same as the lifetime of the bytes reference, and as far as I can tell one should not be able to drop the owned bytes while the typed borrow is still live.

All that being said, it wouldn't be the first time that "upholding rules about how one thinks Rust probably works" has not been the same as "writing Rust without UB". The crate is unfortunately full of this, and marks all of its methods unsafe for that reason (to avoid introducing unsoundness in others' code).