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

Return an error if a decoded slice length doesn't fit into usize #491

Merged

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Jan 25, 2022

Bincode encodes a slice length, which is an usize, as an u64. When such an encoded slice length needs to be decoded it again uses an u64 but critically it truncates it into an usize.

An usize is architecture dependent, it is the size how many bytes it takes to reference any location in memory. The most common sizes for an usize are 64, 32, and 16 bit.

This might lead to silent data loss if the architecture that encoded the slice differs from the architecture that decoded the slice, i.e. if we go from a 64 bit architecture to a 32 or 16 bit one.

Since bincode aims to be suitable for storage, aiming to support the exchange of data between different architectures silently truncating such slice lenghts should be avoided.

This patch changes the behaviour to error out if we try to decode an slice lenght that can't fit into the current usize type.

Bincode encodes a slice length, which is an usize, as an u64. When such
an encoded slice length needs to be decoded it again uses an u64 but
critically it truncates it into an usize.

An usize is architecture dependent, it is the size how many bytes it takes to
reference any location in memory. The most common sizes for an usize are 64, 32,
and 16 bit.

This might lead to silent data loss if the architecture that encoded the
slice differs from the architecture that decoded the slice, i.e. if we
go from a 64 bit architecture to a 32 or 16 bit one.

Since bincode aims to be suitable for storage, aiming to support the
exchange of data between different architectures silently truncating
such slice lenghts should be avoided.

This patch changes the behaviour to error out if we try to decode an
slice lenght that can't fit into the current usize type.
@poljar poljar force-pushed the poljar/check-if-len-fits-in-usize branch from 12195db to a4c92d5 Compare January 25, 2022 10:41
@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #491 (9fe5d1b) into trunk (e036a40) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 9fe5d1b differs from pull request most recent head c6c9b3d. Consider uploading reports for the commit c6c9b3d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            trunk     #491      +/-   ##
==========================================
+ Coverage   69.26%   69.32%   +0.05%     
==========================================
  Files          39       39              
  Lines        2925     2927       +2     
==========================================
+ Hits         2026     2029       +3     
+ Misses        899      898       -1     
Impacted Files Coverage Δ
src/error.rs 0.00% <ø> (ø)
src/de/impls.rs 79.79% <100.00%> (+0.42%) ⬆️
src/de/mod.rs 80.00% <100.00%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e036a40...c6c9b3d. Read the comment docs.

@VictorKoenders
Copy link
Contributor

Quick scan through the source code to see if we have some more cases where we cast to usize, found some:

Can you add this check to the 2 places above?

@poljar
Copy link
Contributor Author

poljar commented Jan 25, 2022

Can you add this check to the 2 places above?

Did so, I also renamed the error variant since it isn't length specific anymore.

@VictorKoenders VictorKoenders merged commit d54dcb8 into bincode-org:trunk Jan 25, 2022
@VictorKoenders
Copy link
Contributor

Thanks!

@poljar poljar deleted the poljar/check-if-len-fits-in-usize branch January 25, 2022 17:00
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

Successfully merging this pull request may close these issues.

None yet

3 participants