tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
446 stars 82 forks source link

BytesReader::read_packed_fixed is unsound because it doesn't respect alignment #215

Closed saethlin closed 1 year ago

saethlin commented 2 years ago

read_packed_fixed converts a &[u8] to a &[T] without checking that the incoming reference satisfies the alignment of the outgoing reference. Since references guarantee that they are properly aligned and both this function and MessageRead are safe, this is unsound. https://github.com/tafia/quick-protobuf/blob/d977371e05170a016f03a80512b2a925468e3a1a/quick-protobuf/src/reader.rs#L387-L401

Normally I'd submit a PR to fix this, but I'm not sure what the right approach is here. This function could be removed, but the alternatives probably impose a performance penalty, and that would be a breaking change. If an alignment check is added to this function, we'd need a competent error to return. Error isn't marked #[non_exhaustive] so adding a variant to it is also a breaking change. And if a check is added, I'm sure users would start getting errors that they don't expect or realistically can't handle, because that would turn "UB but works on my machine" into a hard error.