servo / rust-url

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

use checked addition to not panic in debug build #877

Closed Skgland closed 8 months ago

Skgland commented 8 months ago

used checked addition so this does not panic in debug builds

related to issue #870

not sure if #870 is completely resolved as there are other comments indicating a possible overflow condition that I did not understand and as such did not fix

Skgland commented 8 months ago

Its also probably not ideal tha the fixed code and the test are in different crates, but I didn't know how to separate the punycode from the url and how to add a test in the punycode crate as that appears to use a custom testing framework

Skgland commented 8 months ago

not sure if #870 is completely resolved as there are other comments indicating a possible overflow condition that I did not understand and as such did not fix

I think the line I was refering to is fine: min_code_point - code_point > (u32::MAX - delta) / (processed + 1)

min_code_point should be lager than code_point due to the iterator filter c >= code_point as such this can't underflow on min_code_point - code_point

u32::MAX - delta cannot underflow either as delta can't be larger than u32:MAX

process + 1 can't overflow as process < input_length per the while condition

process + 1 is not 0 as process is unsigned, so no division by zero

Skgland commented 8 months ago

Restructured changes and moved test to the idna creat where it belongs in my opinion.

Skgland commented 8 months ago

Rebase again as I accidentally hardcoded the string in the Bad Punycode test instead of using the value(s) from bad_punycode_tests.json.

Skgland commented 8 months ago

This should now be ready for review again.

codecov[bot] commented 8 months ago

Codecov Report

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

Comparison is base (e39c9a2) 81.81% compared to head (336623e) 81.84%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #877 +/- ## ========================================== + Coverage 81.81% 81.84% +0.03% ========================================== Files 20 20 Lines 3514 3531 +17 ========================================== + Hits 2875 2890 +15 - Misses 639 641 +2 ``` | [Files](https://app.codecov.io/gh/servo/rust-url/pull/877?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/877?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-aWRuYS9zcmMvcHVueWNvZGUucnM=) | `85.44% <100.00%> (+0.44%)` | :arrow_up: | | [idna/tests/punycode.rs](https://app.codecov.io/gh/servo/rust-url/pull/877?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo#diff-aWRuYS90ZXN0cy9wdW55Y29kZS5ycw==) | `80.39% <78.94%> (-0.86%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/servo/rust-url/pull/877/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.

Skgland commented 8 months ago

Fixed added test to not increase msrv beyond 1.56 (I think this change also increases readability of the test) and fixed unrelated clippy lint. Ready for review again.

Skgland commented 8 months ago

Now with fixed formatting. Ready for review again, sorry for the noise.

Skgland commented 8 months ago

I messed up my rebase, one moment.

Skgland commented 8 months ago

Ok, now the rebase should be fixed.