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

skip some duplicate checks when deserializing fixed types #84

Merged
merged 1 commit into from
Jul 8, 2023

Conversation

ralexstokes
Copy link
Owner

from Oak Security audit:

When deserializing Array, List, and Vector objects, depending on whether they are fixed or variable, one of two functions is called within deserialize_homogeneous_composite - deserialize_variable_homogeneous_composite or deserialize_fixed_homogeneous_composite.
The latter is called when the type is fixed, and the corresponding deserialize functions have already validated the length of the object to be a multiple of the default value of that type obtained using T::size_hint. An example for the Array type is the validation performed in ssz-rs/src/array.rs:51-65.
Nevertheless, the deserialize_fixed_homogeneous_composite function in lines 71-78 validates whether the modulo of the length of the deserialized object and the default size for its type is different from zero. Bearing in mind the fact that in the previous step this size was multiplied by N, it means that consequently there is no possibility that the modulo will be different from zero. Ultimately, it is therefore a redundant piece of code.

@@ -188,6 +188,18 @@ where
T: SimpleSerialize,
{
fn deserialize(encoding: &[u8]) -> Result<Self, DeserializeError> {
if !T::is_variable_size() {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note, this check is not entirely redundant but if we want to remove the duplicity for Vector and array types, then we can do something like this

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.08 🎉

Comparison is base (b872969) 75.26% compared to head (42b7852) 75.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
+ Coverage   75.26%   75.34%   +0.08%     
==========================================
  Files          18       18              
  Lines         857      860       +3     
==========================================
+ Hits          645      648       +3     
  Misses        212      212              
Impacted Files Coverage Δ
ssz-rs/src/de.rs 78.18% <100.00%> (-1.82%) ⬇️
ssz-rs/src/list.rs 77.38% <100.00%> (+2.38%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ralexstokes ralexstokes merged commit b73ea4b into main Jul 8, 2023
5 checks passed
@ralexstokes ralexstokes deleted the deser-fixed-comp branch July 8, 2023 21:59
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.

1 participant