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

Add unit test for multihash panic in multiformats/multihash #1409

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

mattheworris
Copy link
Collaborator

@mattheworris mattheworris commented Apr 14, 2023

Goal

The goal of this PR is to add a unit test in the messages pallet to detect a panic on some types of bad cids.
multihash v0.18.1 fixes an issue with calling unwrap() on certain types of errors in a no-std environment.
multihash is included by the cid crate used for IPFS messages.
[Cargo.lock was updated by another PR before this was merged, so v0.18.1 is already there]

Closes #1233

Further updates in progress #1417
multiformats/rust-cid#135

@mattheworris mattheworris self-assigned this Apr 14, 2023
@mattheworris mattheworris marked this pull request as ready for review April 14, 2023 23:42
@mattheworris mattheworris requested a review from wilwade as a code owner April 14, 2023 23:42
@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Merging #1409 (dfea657) into main (44a40c5) will decrease coverage by 1.54%.
The diff coverage is 58.61%.

@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
- Coverage   70.89%   69.35%   -1.54%     
==========================================
  Files          47       56       +9     
  Lines        4356     4977     +621     
==========================================
+ Hits         3088     3452     +364     
- Misses       1268     1525     +257     
Impacted Files Coverage Δ
common/primitives/src/lib.rs 100.00% <ø> (ø)
...requency-tx-payment/src/capacity_stable_weights.rs 0.00% <0.00%> (ø)
pallets/handles/src/runtime-api/src/lib.rs 4.54% <4.54%> (ø)
common/primitives/src/handles.rs 30.76% <30.76%> (ø)
pallets/handles/src/lib.rs 53.23% <53.23%> (ø)
pallets/handles/src/handles_signed_extension.rs 56.52% <56.52%> (ø)
pallets/handles/src/rpc/src/lib.rs 87.80% <87.80%> (ø)
pallets/handles/src/handles-utils/src/converter.rs 100.00% <100.00%> (ø)
pallets/handles/src/handles-utils/src/lib.rs 100.00% <100.00%> (ø)
pallets/handles/src/handles-utils/src/suffix.rs 100.00% <100.00%> (ø)
... and 2 more

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

Cargo.lock Outdated
@@ -859,7 +909,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "fd94671561e36e4e7de75f753f577edafb0e7c05d6e4547229fdf7938fbcd2c3"
dependencies = [
"core2",
"multihash 0.18.0",
"multihash 0.18.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the crate version that fixes the panic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest bumping the dependency in Cargo.toml as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it is an indirect dependency we should be careful with updating the lock file

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will technically work, but it might be nice to submit a PR to https://github.com/multiformats/rust-cid/blob/master/Cargo.toml to bump the actual dependency there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we could bump our cid dependency in Cargo.toml.
Meanwhile, approving--but maybe file a chore to follow up once the cid crate bumps the underlying dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if it is an indirect dependency we should be careful with updating the lock file

Yeah, the Cargo.lock file reflects the changes from executing cargo update, I did not edit the file directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@saraswatpuneet , it's not a manual update of the lock file; I think Cargo updated the indirect dependency based on the rules, now that multihash has published a version with the fix. But my previous comments stand--we should encourage the cid crate to update their dependency, and then update ours explicitly.

@mattheworris mattheworris force-pushed the bug/validate_cid_panic branch from 8c68ea2 to dfea657 Compare April 17, 2023 19:04
@mattheworris mattheworris merged commit 5761306 into main Apr 17, 2023
@mattheworris mattheworris deleted the bug/validate_cid_panic branch April 17, 2023 19:39
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.

[Bug] validate_cid can cause a panic
4 participants