Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserializing corrupt bytes to Vec panics on low-memory devices #135

Open
isikkema opened this issue Apr 2, 2024 · 2 comments
Open

Deserializing corrupt bytes to Vec panics on low-memory devices #135

isikkema opened this issue Apr 2, 2024 · 2 comments

Comments

@isikkema
Copy link
Contributor

isikkema commented Apr 2, 2024

If the size portion of a serialized Vec gets corrupted, it can lead to postcard expecting the size to be much much larger than it actually is.
Example on Teensy 4.0:

from_bytes([8, 255, 255, 255, 0, 0, 0, 0, 0]) => /* Vec with capacity of 8 and elements [255, 255, 255, 0, 0, 0, 0, 0] */
from_bytes([(1 << 7) | 8, 255, 255, 255, 0, 0, 0, 0, 0]) => /* Vec with capacity of 268,435,336 and elements ??? */

Serde ends up preventing panics on most devices because it limits the amount of pre-allocated bytes to 1,048,576, but this is still too large for many low-memory embedded devices.

I propose adding the following function to Flavor:

/// Returns the number of bytes remaining in the message, if known.
fn size_hint(&self) -> Option<usize>;

and using it in SeqAccess like so:

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

This removes the pre-alloc optimization in cases where postcard expects more elements than there are bytes in the remaining message. This will likely result in reduced performance when the de-optimization condition is met except in the case of zero-sized types. However, it will reduce the likelihood of allocation panics, instead returning Err(Error::DeserializeUnexpectedEnd).

@jamesmunns
Copy link
Owner

Hey @isikkema, is this specifically with alloc/std Vec?

I think your suggestions are reasonable, though there may be some edge cases (vec of ZSTs? which is silly but possible).

I'd be open to a PR/proposal that:

  • added the default trait method you suggest on the deser flavor
  • added a feature for using the size hint like this

It's likely this wouldn't happen before #128, so I could also consider a more invasive/breaking change if you feel strongly about that.

@isikkema
Copy link
Contributor Author

isikkema commented Apr 2, 2024

Yes, this is with alloc Vec. Heapless Vec doesn't attempt this optimization.

Vecs of ZSTs aren't affected because it ends up pre-allocating 0 bytes regardless, but I'll add in some tests anyways.

The optimization code is in the serde crate itself. The only way to go any further here is to either know the size of the remaining byte message in the serde crate context or to know the Vec element type in the SeqAccess or Deserializer contexts. All in all, I think this is the best solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants