servo / rust-url

URL parser for Rust
https://docs.rs/url/
Apache License 2.0
1.27k stars 317 forks source link

Fix another overflow in punycode encode_into #880

Closed Skgland closed 7 months ago

Skgland commented 7 months ago

It was possible to panic due to an overflow in build with panic on overflow (i.e. debug builds), if the input iterator contained more than u32::MAX elements. Which can happen an systems with target_pointer_width > 32 if a hugh string or char slice is passed to encode or encode_str respectively.

Commit 1. adds a failing test Commit 2. fixes the implementation so that the test passes Commit 3. adds some early detection to encode and encode_str to not waisted work allocating the String and iterating the input

codecov[bot] commented 7 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (912d716) 81.85% compared to head (9b16473) 81.75%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #880 +/- ## ========================================== - Coverage 81.85% 81.75% -0.10% ========================================== Files 20 20 Lines 3532 3536 +4 ========================================== Hits 2891 2891 - Misses 641 645 +4 ``` | [Files](https://app.codecov.io/gh/servo/rust-url/pull/880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) | Coverage Δ | | |---|---|---| | [idna/src/punycode.rs](https://app.codecov.io/gh/servo/rust-url/pull/880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-aWRuYS9zcmMvcHVueWNvZGUucnM=) | `83.95% <50.00%> (-1.50%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/servo/rust-url/pull/880/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.