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

Adjust Punycode overflow checks #949

Merged
merged 1 commit into from
Jul 1, 2024
Merged

Conversation

hsivonen
Copy link
Collaborator

@hsivonen hsivonen commented Jul 1, 2024

  • The change made in 1.0.0 incorrectly assumed that the input length limit removed the need to do overflow check when decoding. Now the internal-caller length limit is taken as a permission to skip overflow checks only when encoding.
  • The RFC gives overflow checking pre-flight math for languages like that don't have checked math. Since Rust does, the code now uses checked_add and checked_mul instead of pre-flight when overflow checks are performed.

* The change made in 1.0.0 incorrectly assumed that the input length
  limit removed the need to do overflow check when decoding. Now the
  internal-caller length limit is taken as a permission to skip
  overflow checks only when encoding.
* The RFC gives overflow checking pre-flight math for languages like
  that don't have checked math. Since Rust does, the code now uses
  checked_add and checked_mul instead of pre-flight when overflow
  checks are performed.
@hsivonen hsivonen requested a review from Manishearth July 1, 2024 17:28
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (idna-v1x@dcfbed3). Learn more about missing BASE report.

Files Patch % Lines
idna/src/punycode.rs 95.65% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             idna-v1x     #949   +/-   ##
===========================================
  Coverage            ?   79.89%           
===========================================
  Files               ?       23           
  Lines               ?     4227           
  Branches            ?        0           
===========================================
  Hits                ?     3377           
  Misses              ?      850           
  Partials            ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Would be nice to also have tests, but that's ok in a followup

@Manishearth Manishearth merged commit 5019dd5 into servo:idna-v1x Jul 1, 2024
14 checks passed
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.

2 participants