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

Enforce string limits for deserialization #4567

Merged
merged 9 commits into from
Jun 12, 2024
Merged

Enforce string limits for deserialization #4567

merged 9 commits into from
Jun 12, 2024

Conversation

zbuc
Copy link
Contributor

@zbuc zbuc commented Jun 6, 2024

Describe your changes

This follows the suggestion from @hdevalence in #3620 (comment) to add validation of user-supplied String lengths to the various TryFrom protobuf deserialization implementations.

The user-supplied String values concerned are found within the governance and staking crates, concerning governance proposals and validator definitions.

This is consensus breaking as historic data may not comply with the new limitations. Thus there may be unexpected panics when trying to retrieve and deserialize data from storage.

An additional migration has been included to truncate any existing data that does not adhere to the new limits.

The strings are configured to use byte length limits rather than character length limits. This seems favorable because values will be guaranteed to fit into fixed-size buffers of the byte length limit for the field. Code based on rust-lang/rust#93743 was pulled in to perform the truncation at UTF-8 character boundaries.

Issue ticket number and link

Closes #3620

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

Relative to the proposed migration structure, the limits lgtm

crates/bin/pd/src/migrate.rs Outdated Show resolved Hide resolved
@zbuc zbuc force-pushed the 3620_string_limits branch from e281cbd to 97cbfd3 Compare June 10, 2024 18:45
@zbuc zbuc marked this pull request as ready for review June 10, 2024 18:45
@zbuc zbuc requested a review from conorsch June 10, 2024 18:49
@zbuc zbuc added consensus-breaking breaking change to execution of on-chain data state-breaking breaking change to on-chain data migration A pull request that contains a migration A-staking Area: Design and implementation of staking and delegation A-governance Area: Governance _P-V1 Priority: slated for V1 release labels Jun 10, 2024
@zbuc zbuc requested a review from erwanor June 10, 2024 18:55
@zbuc zbuc merged commit defd51b into main Jun 12, 2024
16 checks passed
@zbuc zbuc deleted the 3620_string_limits branch June 12, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-governance Area: Governance A-staking Area: Design and implementation of staking and delegation consensus-breaking breaking change to execution of on-chain data migration A pull request that contains a migration _P-V1 Priority: slated for V1 release state-breaking breaking change to on-chain data
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

penumbra: review user supplier string fields in messages
3 participants