Skip to content

Commit

Permalink
Fix Vec pre-alloc panic (#136)
Browse files Browse the repository at this point in the history
* Add allocvec tests

* Add size_hint() to Flavor

* Don't give SecAccess size hint if len > remaining bytes

* Impl size_hint() for feature-gated flavors

* Add default size_hint() impl

* Add implementation notes doc comments for size_hint()

---------

Co-authored-by: Isaac Sikkema <sikkemaic@ornl.gov>
  • Loading branch information
isikkema and Isaac Sikkema committed Apr 3, 2024
1 parent 393e18a commit 6338601
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/de/deserializer.rs
Expand Up @@ -168,7 +168,10 @@ impl<'a, 'b: 'a, F: Flavor<'b>> serde::de::SeqAccess<'b> for SeqAccess<'a, 'b, F

#[inline]
fn size_hint(&self) -> Option<usize> {
Some(self.len)
match self.deserializer.flavor.size_hint() {
Some(size) if size < self.len => None,
_ => Some(self.len),
}
}
}

Expand Down
46 changes: 46 additions & 0 deletions src/de/flavors.rs
Expand Up @@ -95,6 +95,27 @@ pub trait Flavor<'de>: 'de {
/// Obtain the next byte for deserialization
fn pop(&mut self) -> Result<u8>;

/// Returns the number of bytes remaining in the message, if known.
///
/// # Implementation notes
///
/// It is not enforced that this number is exactly correct.
/// A flavor may yield less or more bytes than the what is hinted at by
/// this function.
///
/// `size_hint()` is primarily intended to be used for optimizations such as
/// reserving space for deserialized items, but must not be trusted to
/// e.g., omit bounds checks in unsafe code. An incorrect implementation of
/// `size_hint()` should not lead to memory safety violations.
///
/// That said, the implementation should provide a correct estimation,
/// because otherwise it would be a violation of the trait’s protocol.
///
/// The default implementation returns `None` which is correct for any flavor.
fn size_hint(&self) -> Option<usize> {
None
}

/// Attempt to take the next `ct` bytes from the serialized message
fn try_take_n(&mut self, ct: usize) -> Result<&'de [u8]>;

Expand Down Expand Up @@ -142,6 +163,11 @@ impl<'de> Flavor<'de> for Slice<'de> {
}
}

#[inline]
fn size_hint(&self) -> Option<usize> {
Some((self.end as usize) - (self.cursor as usize))
}

#[inline]
fn try_take_n(&mut self, ct: usize) -> Result<&'de [u8]> {
let remain = (self.end as usize) - (self.cursor as usize);
Expand Down Expand Up @@ -188,6 +214,11 @@ pub mod io {
}
}

#[inline]
fn size(&self) -> usize {
(self.end as usize) - (self.cursor as usize)
}

#[inline]
fn take_n(&mut self, ct: usize) -> Result<&'de mut [u8]> {
let remain = (self.end as usize) - (self.cursor as usize);
Expand Down Expand Up @@ -254,6 +285,11 @@ pub mod io {
Ok(val[0])
}

#[inline]
fn size_hint(&self) -> Option<usize> {
Some(self.buff.size())
}

#[inline]
fn try_take_n(&mut self, ct: usize) -> Result<&'de [u8]> {
let buff = self.buff.take_n(ct)?;
Expand Down Expand Up @@ -315,6 +351,11 @@ pub mod io {
Ok(val[0])
}

#[inline]
fn size_hint(&self) -> Option<usize> {
Some(self.buff.size())
}

#[inline]
fn try_take_n(&mut self, ct: usize) -> Result<&'de [u8]> {
let buff = self.buff.take_n(ct)?;
Expand Down Expand Up @@ -406,6 +447,11 @@ pub mod crc {
}
}

#[inline]
fn size_hint(&self) -> Option<usize> {
self.flav.size_hint()
}

#[inline]
fn try_take_n(&mut self, ct: usize) -> Result<&'de [u8]> {
match self.flav.try_take_n(ct) {
Expand Down
39 changes: 39 additions & 0 deletions src/de/mod.rs
Expand Up @@ -566,3 +566,42 @@ mod test_heapless {
assert_eq!(remain.len(), 0);
}
}

#[cfg(any(feature = "alloc", feature = "use-std"))]
#[cfg(test)]
mod test_alloc {
extern crate alloc;

use super::*;

use alloc::vec;
use serde::Deserialize;

#[derive(Debug, Deserialize, PartialEq)]
struct ZSTStruct;

#[test]
fn zst_vec() {
assert_eq!(from_bytes(&[3]), Ok(vec![ZSTStruct, ZSTStruct, ZSTStruct]));

assert_eq!(
from_bytes(&[4]),
Ok(vec![ZSTStruct, ZSTStruct, ZSTStruct, ZSTStruct])
);
}

#[test]
fn vec() {
assert_eq!(
from_bytes::<Vec<u8>>(&[8, 255, 255, 255, 0, 0, 0, 0, 0]),
Ok(vec![255, 255, 255, 0, 0, 0, 0, 0])
);

// This won't actually prove anything since tests will likely always be
// run on devices with larger amounts of memory, but it can't hurt.
assert_eq!(
from_bytes::<Vec<u8>>(&[(1 << 7) | 8, 255, 255, 255, 0, 0, 0, 0, 0]),
Err(Error::DeserializeUnexpectedEnd)
);
}
}

0 comments on commit 6338601

Please sign in to comment.