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

Use decode_all for decoding cross contract call result #1810

Merged
merged 14 commits into from
Jul 5, 2023
Merged

Conversation

ascjones
Copy link
Collaborator

Related to GHSA-853p-5678-hv8f.

decode can still succeed if there are bytes remaining. In the above security advisory the issue was that the bytes for Result<T> were successfully decoded into a T but with the incorrect value, because Result encoding has an extra byte prefix.

If we had been using decode_all instead, which fails if there are any bytes remaining in the input, this would have been discovered earlier.

This provides an extra level of safety since it avoids e.g. truncation of values: preventing e.g. an i32 returned by the callee being decoded into an i8 (see integration-test as part of this PR).

Note there are other uses of decode which could be replaced by decode_all (see #1804), but we can tackle those separately.

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Looks good, but why can't we have the same generic integration test for the delegate call?

integration-tests/call-builder-return-value/lib.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2023

Codecov Report

Merging #1810 (248d1a9) into master (14c850c) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1810      +/-   ##
==========================================
+ Coverage   52.11%   52.16%   +0.04%     
==========================================
  Files         206      206              
  Lines        6656     6656              
==========================================
+ Hits         3469     3472       +3     
+ Misses       3187     3184       -3     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

LGTM!

@ascjones ascjones merged commit 255fc20 into master Jul 5, 2023
22 checks passed
@ascjones ascjones deleted the aj/decode-all branch July 5, 2023 08:04
@ascjones ascjones mentioned this pull request Sep 8, 2023
@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
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

4 participants