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

Fix IPv4 octet and mask length overflow (#57) #60

Merged
merged 1 commit into from Nov 4, 2021

Conversation

TravisCardwell
Copy link
Contributor

The dig function in Data.IP.Addr uses an Int accumulator that can overflow while parsing. It is used to parse IPv4 octets as well as IPv4 and IPv6 mask lengths. This change replaces the dig function with two special-purpose functions:

The octet function parses IPv4 octets. It checks the maximum bound while parsing so that overflow is not possible.

The maskLen function parses mask lengths. The maximum bound is passed as an argument (32 for IPv4 and 128 for IPv6), and it checks the maximum bound while parsing so that overflow is not possible. The use of option is refactored to only check for the / character, so that an invalid mask length after the / character can cause a parse failure.

Note that these functions include the following performance optimizations:

  • The ord function is used instead of digitToInt, as the range of characters is already checked.
  • The parameters to the go helper function are given (bang) strictness annotations to help GHC determine that the Char and Int values can be unboxed.

An unfortunate side effect of this change is that some error messages change.

The `dig` function in `Data.IP.Addr` uses an `Int` accumulator that
can overflow while parsing.  It is used to parse IPv4 octets as well
as IPv4 and IPv6 mask lengths.  This change replaces the `dig`
function with two special-purpose functions:

The `octet` function parses IPv4 octets.  It checks the maximum bound
while parsing so that overflow is not possible.

The `maskLen` function parses mask lengths.  The maximum bound is
passed as an argument (32 for IPv4 and 128 for IPv6), and it checks
the maximum bound while parsing so that overflow is not possible.  The
use of `option` is refactored to only check for the `/` character, so
that an invalid mask length after the `/` character can cause a parse
failure.

Note that these functions include the following performance
optimizations:

* The `ord` function is used instead of `digitToInt`, as the range of
  characters is already checked.
* The parameters to the `go` helper function are given (bang)
  strictness annotations to help GHC determine that the `Char` and
  `Int` values can be unboxed.

An unfortunate side effect of this change is that some error messages
change.
@TravisCardwell
Copy link
Contributor Author

I realized that my commit included the digit range checks even though oneOf['1'..'9'] and many digit guarantee that all characters are digits. I removed the range checks, squashing the commit to keep the commit history clean.

@kazu-yamamoto kazu-yamamoto self-requested a review November 4, 2021 00:44
Copy link
Owner

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

LGTM

@kazu-yamamoto kazu-yamamoto merged commit 8c703cc into kazu-yamamoto:master Nov 4, 2021
@kazu-yamamoto
Copy link
Owner

A new version has been released.

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.

None yet

2 participants