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

Parsing of non-UTF8 CommitmentPrefix is incorrect #1275

Closed
Farhad-Shabani opened this issue Jul 10, 2024 · 1 comment · Fixed by #1273
Closed

Parsing of non-UTF8 CommitmentPrefix is incorrect #1275

Farhad-Shabani opened this issue Jul 10, 2024 · 1 comment · Fixed by #1273
Assignees
Labels
A: bug Admin: something isn't working
Milestone

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Jul 10, 2024

Bug Summary

When performing verify_(non_)membership in the Tendermint light client, we use apply_prefix to construct the merkle path necessary for proof existence checks. The apply_prefix converts the CommitmentPrefix to a string. However, if the prefix is not valid UTF-8, the function cannot handle it properly and outputs an incorrect string in the format of:
<not valid UTF8: {commitment_prefix:?}>

Proposal

Version

=< 0.53.0

@Farhad-Shabani Farhad-Shabani added the A: bug Admin: something isn't working label Jul 10, 2024
@Farhad-Shabani Farhad-Shabani added this to the 0.54.0 milestone Jul 10, 2024
@Farhad-Shabani Farhad-Shabani self-assigned this Jul 10, 2024
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Jul 10, 2024
@Farhad-Shabani
Copy link
Member Author

As for the Debug impl

We can't simply remove or alter the Debug implementation for CommitmentPrefix, as it is used as the type for the prefix field in connection messages. Therefore all the Debug impl up to the MsgEnvelope relies on having a Debug implementation for CommitmentPrefix.

Anyhow, since we plan to move away from the legacy apply_prefix and allow path bytes (#1273), we won't encounter the parsing issue in critical paths.

It also should be noted we can't return an error on the Err variant of Debug, as obtaining a string from messages containing non-UTF8 prefix bytes would result in an error. Therefore, the current Debug implementation looks fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Admin: something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant