Skip to content

Punycode error handling #31977

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

Merged
merged 4 commits into from
Jul 6, 2020
Merged

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented May 22, 2020

This updates our Punycode implementation with error checks. Most of these checks are detailed in RFC3492.

Resolves rdar://63392615

tbkka added 2 commits May 18, 2020 18:19
The `Qo` operator expects to consume a type name and a list (terminated with a `y` empty list marker) from the stack.  After popping the list, it doesn't check whether the stack is empty, so `$syQo` crashes (it pops down to the `y` then tries to pop again).

This PR just adds the obvious check to guard against this.

Resolves rdar://63128307
Fuzz tests have revealed some weaknesses in the error handling of our Punycode implementation used to mangle Unicode identifiers.  A more detailed comparison of the implementation against the algorithm detailed in RFC3492 showed that most of the arithmetic overflow checks were omitted and the ones that were present were handled as success instead of failure.

A typical example:

RFC3492 algorithm:
```
 let w = w * (base - t), fail on overflow
```

Original implementation:
```
 w = w * (base - t);
```

Corrected implementation:
```
 if (w > std::numeric_limits<int>::max() / (base - t))
   return false;
 w = w * (base - t);
```

Resolves rdar://63392615
@tbkka tbkka requested a review from jckarter May 22, 2020 21:20
@tbkka
Copy link
Contributor Author

tbkka commented May 26, 2020

This was based on comparing our implementation to

  1. The pseudocode outline in RFC3492:
    https://tools.ietf.org/html/rfc3492#page-11
  2. The sample C implementation:
    https://tools.ietf.org/html/rfc3492#page-23

Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

@tbkka
Copy link
Contributor Author

tbkka commented Jul 6, 2020

@swift-ci Please test

@tbkka tbkka merged commit 74f2878 into swiftlang:master Jul 6, 2020
@tbkka tbkka deleted the tbkka-rdar63392615-punycode branch October 16, 2020 00:33
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