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 ipv6 parser #4241

Closed
wants to merge 3 commits into from
Closed

Fix ipv6 parser #4241

wants to merge 3 commits into from

Conversation

m-sp
Copy link
Member

@m-sp m-sp commented Jan 23, 2021

should hopefully fix #4145 (review)

but the following test fails:


    "only parse part of an invalid ipv6 address where a single section is shortened (must be 2 or more)" in {
      val invalidIp = "2001:db8::1:1:1:1:1"
      Ipv6Address.fromString(invalidIp).map(_.value) must beLeft
    }

but I don't really see why the ip address is invalid

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

The failing test does parse in non-http4s decoders.

.map { case ((ls: Option[List[Short]], r0: Short), rs) =>
toIpv6(ls.getOrElse(Seq.empty), Seq(r0) ++ rs)
})
.orElse(((h16.repSep0(0, 3, colon).with1 <* doubleColon) ~ h16Colon.repExactlyAs[List[Short]](2).backtrack ~ ls32)
Copy link
Member

Choose a reason for hiding this comment

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

Why (0, 3) instead of (3, 3)?

Copy link
Member Author

Choose a reason for hiding this comment

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

it would fail on e.g. 53f6::1:551b:7bd8

the number of segments after :: are always fixed to a number n. so before the :: we can parse up to 8 - n segments

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. So this is part of the bug fix.

@rossabaker rossabaker added this to In progress in Dotty cross-compilation via automation Jan 23, 2021
@rossabaker
Copy link
Member

Oh, that address was invalid because a :: is supposed to represent multiple groups of zeroes. Strictly, the test was correct, but I don't think it ambiguates anything to accept it.

@m-sp
Copy link
Member Author

m-sp commented Jan 23, 2021

Oh, that address was invalid because a :: is supposed to represent multiple groups of zeroes. Strictly, the test was correct, but I don't think it ambiguates anything to accept it.

I have an alternative fix here #4243 which also handles that case and IMO is a bit easier to understand. let me know which one you prefer and I can resolve the conflicts afterwards

@m-sp m-sp closed this Jan 23, 2021
Dotty cross-compilation automation moved this from In progress to Done Jan 23, 2021
@m-sp m-sp deleted the fix-ipv6-parser branch January 23, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants