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

IPv4 octet parsing subject to Word overflow #74

Closed
TravisCardwell opened this issue Oct 22, 2021 · 6 comments
Closed

IPv4 octet parsing subject to Word overflow #74

TravisCardwell opened this issue Oct 22, 2021 · 6 comments

Comments

@TravisCardwell
Copy link
Contributor

The decodeIPv4TextReader function uses TextRead.decimal to parse Word values. Since Word is a bounded type, it is subject to overflow, which causes some invalid strings to parse successfully.

Example:

λ: import Net.IPv4 (decode)
λ: print $ decode "127.0.0.18446744073709551617"
Just (ipv4 127 0 0 1)

I do not think that this issue is significant, but I figure that I should at least report it.

@andrewthad
Copy link
Member

andrewthad commented Oct 27, 2021

If anyone wants to fix this, I would accept a PR for it.

EDIT: Also, thanks for reporting.

@TravisCardwell
Copy link
Contributor Author

You are welcome!

I just submitted a PR with a fix.

@TravisCardwell
Copy link
Contributor Author

TravisCardwell commented Oct 28, 2021

I went for a hike this afternoon and realized that my comment about digitToInt was inaccurate. (It supports ASCII hex digits.) Checking the source to confirm, I realized that it is best to avoid both isDigit and digitToInt. This leads to slightly better performance: two fewer calls of fromIntegral, three fewer calls of ord, and one less subtraction per parsed character. I updated the pull request, squashing the commits to keep the commit history clean.

If you have any ideas for further improvement or other suggestions, please feel free to let me know, and I would be happy to look into them.

andrewthad pushed a commit that referenced this issue Oct 29, 2021
Add IPv4 octet overflow test and fix IPv4 octet overflow bug
@andrewthad
Copy link
Member

Resolved in #75

TravisCardwell added a commit to TravisCardwell/haskell-ip that referenced this issue Nov 2, 2021
All processed characters are guaranteed to be digits (0~9) through use
of `Char.isDigit`, so the range check in helper function `go` is not
necessary.
@TravisCardwell
Copy link
Contributor Author

I realized that the readOctet function can be optimized further... All processed characters are guaranteed to be digits (~9) through use of Char.isDigit, so the range check in helper function go` is not necessary.

diff --git a/src/Net/IPv4.hs b/src/Net/IPv4.hs
index 40459d7..a020582 100644
--- a/src/Net/IPv4.hs
+++ b/src/Net/IPv4.hs
@@ -771,9 +771,8 @@ readOctet t = do
   where
   go :: Char -> (Int -> Maybe Int) -> Int -> Maybe Int
   go !d !f !n =
-    let d' = Char.ord d - 48
-        n' = n * 10 + d'
-    in  if d' >= 0 && d' <=9 && n' <= 255 then f n' else Nothing
+    let n' = n * 10 + Char.ord d - 48
+    in  if n' <= 255 then f n' else Nothing
 
 stripDecimal :: Text -> Either String Text
 stripDecimal t = case Text.uncons t of

Sorry for not noticing that before! If you would like to commit this optimization, I can create a PR.

@andrewthad
Copy link
Member

Yeah, go ahead and PR. That would be great.

andrewthad pushed a commit that referenced this issue Nov 4, 2021
All processed characters are guaranteed to be digits (0~9) through use
of `Char.isDigit`, so the range check in helper function `go` is not
necessary.
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

No branches or pull requests

2 participants