Repository: servo/rust-url
Pull request #877 (2023-11-03 17:56:17)

use checked addition to not panic in debug build
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: 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:  
> 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

`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: Restructured changes and moved test to idna where it belongs in my opinion.
- The first commit fixes the line endings of `debug_metadata/README.md` as it was always considered dirty and prevented me from rebasing otherwise.
- The second commit adds the test, fails without the third commit.
- The third and last commit fixed the panic in debug builds due to overflow.
Skgland: 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: This should now be ready for review again.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/servo/rust-url/pull/877?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) Report
Attention: `4 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`1158370`)](https://app.codecov.io/gh/servo/rust-url/commit/115837002b4b56be70e3a67813716931c6c88e47?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) 81.42% compared to head [(`d533df4`)](https://app.codecov.io/gh/servo/rust-url/pull/877?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo) 81.81%.
> Report is 2 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
+ Coverage   81.42%   81.81%   +0.39%     
==========================================
  Files          20       20              
  Lines        3516     3531      +15     
==========================================
+ Hits         2863     2889      +26     
+ Misses        653      642      -11     
```


| [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/src/uts46.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-aWRuYS9zcmMvdXRzNDYucnM=) | `94.17% <100.00%> (-0.02%)` | :arrow_down: |
| [url/src/lib.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-dXJsL3NyYy9saWIucnM=) | `75.53% <ø> (+1.46%)` | :arrow_up: |
| [url/src/parser.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-dXJsL3NyYy9wYXJzZXIucnM=) | `81.70% <100.00%> (+0.23%)` | :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)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/servo/rust-url/pull/877?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=servo).

Skgland: Fixed added test to not increase msrv beyond 1.56 and fixed clippy lint.
Ready for review again.
Skgland: Now with fixed formatting.
Ready for review again, sorry for the noise.
Skgland: I messed up my rebase, one moment.

Skgland: Ok, now the rebase should be fixed.


Patch:
diff --git a/debug_metadata/README.md b/debug_metadata/README.md
index ddf038d..dab8de6 100644
--- a/debug_metadata/README.md
+++ b/debug_metadata/README.md
@@ -1,111 +1,111 @@
-## Debugger Visualizers
-
-Many languages and debuggers enable developers to control how a type is
-displayed in a debugger. These are called "debugger visualizations" or "debugger
-views".
-
-The Windows debuggers (WinDbg\CDB) support defining custom debugger visualizations using
-the `Natvis` framework. To use Natvis, developers write XML documents using the natvis
-schema that describe how debugger types should be displayed with the `.natvis` extension.
-(See: https://docs.microsoft.com/en-us/visualstudio/debugger/create-custom-views-of-native-objects?view=vs-2019)
-The Natvis files provide patterns which match type names a description of how to display
-those types.
-
-The Natvis schema can be found either online (See: https://code.visualstudio.com/docs/cpp/natvis#_schema)
-or locally at `<VS Installation Folder>\Xml\Schemas\1033\natvis.xsd`.
-
-The GNU debugger (GDB) supports defining custom debugger views using Pretty Printers.
-Pretty printers are written as python scripts that describe how a type should be displayed
-when loaded up in GDB/LLDB. (See: https://sourceware.org/gdb/onlinedocs/gdb/Pretty-Printing.html#Pretty-Printing)
-The pretty printers provide patterns, which match type names, and for matching
-types, descibe how to display those types. (For writing a pretty printer, see: https://sourceware.org/gdb/onlinedocs/gdb/Writing-a-Pretty_002dPrinter.html#Writing-a-Pretty_002dPrinter).
-
-### Embedding Visualizers
-
-Through the use of the `#[debugger_visualizer]` attribute, the `url` crate can embed
-debugger visualizers into the crate metadata.
-
-Currently the two types of visualizers supported are Natvis and Pretty printers.
-
-For Natvis files, when linking an executable with a crate that includes Natvis files,
-the MSVC linker will embed the contents of all Natvis files into the generated `PDB`.
-
-For pretty printers, the compiler will encode the contents of the pretty printer
-in the `.debug_gdb_scripts` section of the `ELF` generated.
-
-### Testing Visualizers
-
-The `url` crate supports testing debugger visualizers defined for this crate. The entry point for
-these tests are `tests/debugger_visualizer.rs`. These tests are defined using the `debugger_test` and
-`debugger_test_parser` crates. The `debugger_test` crate is a proc macro crate which defines a
-single proc macro attribute, `#[debugger_test]`. For more detailed information about this crate,
-see https://crates.io/crates/debugger_test. The CI pipeline for the `url` crate has been updated
-to run the debugger visualizer tests to ensure debugger visualizers do not become broken/stale.
-
-The `#[debugger_test]` proc macro attribute may only be used on test functions and will run the
-function under the debugger specified by the `debugger` meta item.
-
-This proc macro attribute has 3 required values:
-
-1. The first required meta item, `debugger`, takes a string value which specifies the debugger to launch.
-2. The second required meta item, `commands`, takes a string of new line (`\n`) separated list of debugger
-commands to run.
-3. The third required meta item, `expected_statements`, takes a string of new line (`\n`) separated list of
-statements that must exist in the debugger output. Pattern matching through regular expressions is also
-supported by using the `pattern:` prefix for each expected statement.
-
-#### Example:
-
-```rust
-#[debugger_test(
-    debugger = "cdb",
-    commands = "command1\ncommand2\ncommand3",
-    expected_statements = "statement1\nstatement2\nstatement3")]
-fn test() {
-
-}
-```
-
-Using a multiline string is also supported, with a single debugger command/expected statement per line:
-
-```rust
-#[debugger_test(
-    debugger = "cdb",
-    commands = "
-command1
-command2
-command3",
-    expected_statements = "
-statement1
-pattern:statement[0-9]+
-statement3")]
-fn test() {
-    
-}
-```
-
-In the example above, the second expected statement uses pattern matching through a regular expression
-by using the `pattern:` prefix.
-
-#### Testing Locally
-
-Currently, only Natvis visualizations have been defined for the `url` crate via `debug_metadata/url.natvis`,
-which means the `tests/debugger_visualizer.rs` tests need to be run on Windows using the `*-pc-windows-msvc` targets.
-To run these tests locally, first ensure the debugging tools for Windows are installed or install them following
-the steps listed here, [Debugging Tools for Windows](https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/).
-Once the debugging tools have been installed, the tests can be run in the same manner as they are in the CI
-pipeline.
-
-#### Note
-
-When running the debugger visualizer tests, `tests/debugger_visualizer.rs`, they need to be run consecutively
-and not in parallel. This can be achieved by passing the flag `--test-threads=1` to rustc. This is due to
-how the debugger tests are run. Each test marked with the `#[debugger_test]` attribute launches a debugger
-and attaches it to the current test process. If tests are running in parallel, the test will try to attach
-a debugger to the current process which may already have a debugger attached causing the test to fail.
-
-For example:
-
-```
-cargo test --test debugger_visualizer --features debugger_visualizer -- --test-threads=1
-```
+## Debugger Visualizers
+
+Many languages and debuggers enable developers to control how a type is
+displayed in a debugger. These are called "debugger visualizations" or "debugger
+views".
+
+The Windows debuggers (WinDbg\CDB) support defining custom debugger visualizations using
+the `Natvis` framework. To use Natvis, developers write XML documents using the natvis
+schema that describe how debugger types should be displayed with the `.natvis` extension.
+(See: https://docs.microsoft.com/en-us/visualstudio/debugger/create-custom-views-of-native-objects?view=vs-2019)
+The Natvis files provide patterns which match type names a description of how to display
+those types.
+
+The Natvis schema can be found either online (See: https://code.visualstudio.com/docs/cpp/natvis#_schema)
+or locally at `<VS Installation Folder>\Xml\Schemas\1033\natvis.xsd`.
+
+The GNU debugger (GDB) supports defining custom debugger views using Pretty Printers.
+Pretty printers are written as python scripts that describe how a type should be displayed
+when loaded up in GDB/LLDB. (See: https://sourceware.org/gdb/onlinedocs/gdb/Pretty-Printing.html#Pretty-Printing)
+The pretty printers provide patterns, which match type names, and for matching
+types, descibe how to display those types. (For writing a pretty printer, see: https://sourceware.org/gdb/onlinedocs/gdb/Writing-a-Pretty_002dPrinter.html#Writing-a-Pretty_002dPrinter).
+
+### Embedding Visualizers
+
+Through the use of the `#[debugger_visualizer]` attribute, the `url` crate can embed
+debugger visualizers into the crate metadata.
+
+Currently the two types of visualizers supported are Natvis and Pretty printers.
+
+For Natvis files, when linking an executable with a crate that includes Natvis files,
+the MSVC linker will embed the contents of all Natvis files into the generated `PDB`.
+
+For pretty printers, the compiler will encode the contents of the pretty printer
+in the `.debug_gdb_scripts` section of the `ELF` generated.
+
+### Testing Visualizers
+
+The `url` crate supports testing debugger visualizers defined for this crate. The entry point for
+these tests are `tests/debugger_visualizer.rs`. These tests are defined using the `debugger_test` and
+`debugger_test_parser` crates. The `debugger_test` crate is a proc macro crate which defines a
+single proc macro attribute, `#[debugger_test]`. For more detailed information about this crate,
+see https://crates.io/crates/debugger_test. The CI pipeline for the `url` crate has been updated
+to run the debugger visualizer tests to ensure debugger visualizers do not become broken/stale.
+
+The `#[debugger_test]` proc macro attribute may only be used on test functions and will run the
+function under the debugger specified by the `debugger` meta item.
+
+This proc macro attribute has 3 required values:
+
+1. The first required meta item, `debugger`, takes a string value which specifies the debugger to launch.
+2. The second required meta item, `commands`, takes a string of new line (`\n`) separated list of debugger
+commands to run.
+3. The third required meta item, `expected_statements`, takes a string of new line (`\n`) separated list of
+statements that must exist in the debugger output. Pattern matching through regular expressions is also
+supported by using the `pattern:` prefix for each expected statement.
+
+#### Example:
+
+```rust
+#[debugger_test(
+    debugger = "cdb",
+    commands = "command1\ncommand2\ncommand3",
+    expected_statements = "statement1\nstatement2\nstatement3")]
+fn test() {
+
+}
+```
+
+Using a multiline string is also supported, with a single debugger command/expected statement per line:
+
+```rust
+#[debugger_test(
+    debugger = "cdb",
+    commands = "
+command1
+command2
+command3",
+    expected_statements = "
+statement1
+pattern:statement[0-9]+
+statement3")]
+fn test() {
+    
+}
+```
+
+In the example above, the second expected statement uses pattern matching through a regular expression
+by using the `pattern:` prefix.
+
+#### Testing Locally
+
+Currently, only Natvis visualizations have been defined for the `url` crate via `debug_metadata/url.natvis`,
+which means the `tests/debugger_visualizer.rs` tests need to be run on Windows using the `*-pc-windows-msvc` targets.
+To run these tests locally, first ensure the debugging tools for Windows are installed or install them following
+the steps listed here, [Debugging Tools for Windows](https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/).
+Once the debugging tools have been installed, the tests can be run in the same manner as they are in the CI
+pipeline.
+
+#### Note
+
+When running the debugger visualizer tests, `tests/debugger_visualizer.rs`, they need to be run consecutively
+and not in parallel. This can be achieved by passing the flag `--test-threads=1` to rustc. This is due to
+how the debugger tests are run. Each test marked with the `#[debugger_test]` attribute launches a debugger
+and attaches it to the current test process. If tests are running in parallel, the test will try to attach
+a debugger to the current process which may already have a debugger attached causing the test to fail.
+
+For example:
+
+```
+cargo test --test debugger_visualizer --features debugger_visualizer -- --test-threads=1
+```
diff --git a/idna/src/punycode.rs b/idna/src/punycode.rs
index faef379..8d26ce2 100644
--- a/idna/src/punycode.rs
+++ b/idna/src/punycode.rs
@@ -269,10 +269,7 @@ where
         for c in input.clone() {
             let c = c as u32;
             if c < code_point {
-                delta += 1;
-                if delta == 0 {
-                    return Err(()); // Overflow
-                }
+                delta = delta.checked_add(1).ok_or(())?;
             }
             if c == code_point {
                 // Represent delta as a generalized variable-length integer:


Test patch:
diff --git a/idna/tests/bad_punycode_tests.json b/idna/tests/bad_punycode_tests.json
new file mode 100644
index 0000000..c011529
--- /dev/null
+++ b/idna/tests/bad_punycode_tests.json
@@ -0,0 +1,6 @@
+[
+    {
+        "description": "issue 870",
+        "decoded": "                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           񠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                󠀷                                                                                                                                                                                                                              󠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  󠀷                                                                                                                                                                                                                              󠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             󠀷                                                                                                                                                                                                                               󠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      󠀷                                                                                                   󠀷                                                                                                                                                                                                                                                                                                                                                                                                                           󠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                󠀷                                                                                                                                                                                                                                              󠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             󠀷                                                                                                                                                                                  󠀷                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              21日       ᄢ              21日       㩴               ᄢ              21日       ᄢ             "
+    }
+]
diff --git a/idna/tests/punycode.rs b/idna/tests/punycode.rs
index 9567413..2867e49 100644
--- a/idna/tests/punycode.rs
+++ b/idna/tests/punycode.rs
@@ -10,6 +10,7 @@ use crate::test::TestFn;
 use idna::punycode::{decode, encode_str};
 use serde_json::map::Map;
 use serde_json::Value;
+use std::panic::catch_unwind;
 use std::str::FromStr;
 
 fn one_test(decoded: &str, encoded: &str) {
@@ -39,6 +40,15 @@ fn one_test(decoded: &str, encoded: &str) {
     }
 }
 
+fn one_bad_test(encode: &str) {
+    let result = catch_unwind(|| encode_str(encode));
+    assert!(
+        matches!(&result, Ok(None)),
+        "Should neither panic nor return Some result, but got {:?}",
+        result
+    )
+}
+
 fn get_string<'a>(map: &'a Map<String, Value>, key: &str) -> &'a str {
     match map.get(&key.to_string()) {
         Some(Value::String(s)) => s,
@@ -74,4 +84,31 @@ pub fn collect_tests<F: FnMut(String, TestFn)>(add_test: &mut F) {
         }
         other => panic!("{:?}", other),
     }
+
+    match Value::from_str(include_str!("bad_punycode_tests.json")) {
+        Ok(Value::Array(tests)) => {
+            for (i, test) in tests.into_iter().enumerate() {
+                match test {
+                    Value::Object(o) => {
+                        let test_name = {
+                            let desc = get_string(&o, "description");
+                            if desc.is_empty() {
+                                format!("Bad Punycode {}", i + 1)
+                            } else {
+                                format!("Bad Punycode {}: {}", i + 1, desc)
+                            }
+                        };
+                        add_test(
+                            test_name,
+                            TestFn::DynTestFn(Box::new(move || {
+                                one_bad_test(get_string(&o, "decoded"))
+                            })),
+                        )
+                    }
+                    _ => panic!(),
+                }
+            }
+        }
+        other => panic!("{:?}", other),
+    }
 }