Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Don't panic if extra_data is longer than VANITY_LENGTH #10682

Merged
merged 2 commits into from
May 22, 2019
Merged

Don't panic if extra_data is longer than VANITY_LENGTH #10682

merged 2 commits into from
May 22, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented May 21, 2019

Seems like extra_data().len() can be longer than 32 (was 39 here) so this fixes that panic.

@dvdplm
Don't panic if extra_data is longer than VANITY_LENGTH
1a3be05 
Seems like `extra_data().len()` can be longer than 32 (was 39 here) so this fixes that panic.
@dvdplm dvdplm requested a review from niklasad1 May 21, 2019 20:13
@dvdplm dvdplm self-assigned this May 21, 2019
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels May 21, 2019
niklasad1
niklasad1 approved these changes May 22, 2019
View reviewed changes
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice, but it will only panic in debug mode and wrap in release mode

@dvdplm
Copy link
Collaborator Author

dvdplm commented May 22, 2019 •
edited

wrap in release mode

Isn't that potentially even worse though?

EDIT: I'm wrong, if it wraps it's innocuous.

@dvdplm dvdplm added A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). and removed B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels May 22, 2019
@niklasad1
Copy link
Collaborator

Isn't that potentially even worse though?

Yes, that is correct for example if len == 39 then it will wrap to a large value and if the will succeed!

Could potentially cast it to an isize instead but then it possible to the cast the usize to a negative value and it will be addition instead!

Thus, saturating_sub is all good :)

@ordian ordian added this to the 2.6 milestone May 22, 2019
@ordian ordian merged commit 26d1303 into master May 22, 2019
@ordian ordian deleted the dp/fix/populate_from_parent-panic branch May 22, 2019 10:35
dvdplm added a commit that referenced this pull request May 22, 2019
@dvdplm
Merge branch 'master' into dp/chore/switch-to-new-parity-crypto
3d62cb0 
* master:
  Don't panic if extra_data is longer than VANITY_LENGTH (#10682)
  docs: evmbin - Update Rust docs (#10658)
  Remove annoying compiler warnings (#10679)
  Reset blockchain properly (#10669)
dvdplm added a commit that referenced this pull request May 23, 2019
@dvdplm
Merge branch 'master' into dp/fix/move-step-service-into-clique
70b1e0c 
* master:
  add_sync_notifier in EthPubSubClient holds on to a Client for too long (#10689)
  Don't panic if extra_data is longer than VANITY_LENGTH (#10682)
  docs: evmbin - Update Rust docs (#10658)
  Remove annoying compiler warnings (#10679)
  Reset blockchain properly (#10669)
ordian added a commit that referenced this pull request May 23, 2019
@ordian
Merge branch 'master' into ao-upgrade-ethereum-types
3b9f715 
* master:
  add_sync_notifier in EthPubSubClient holds on to a Client for too long (#10689)
  Don't panic if extra_data is longer than VANITY_LENGTH (#10682)
  docs: evmbin - Update Rust docs (#10658)
  Remove annoying compiler warnings (#10679)
  Reset blockchain properly (#10669)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ascjones ascjones ascjones approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Assignees

@dvdplm dvdplm

Labels
A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update).
Projects
None yet
Milestone
2.6
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dvdplm @niklasad1 @ascjones @ordian