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 bug in DNSSD IPv6 address string conversion. #14

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

torinmr
Copy link
Contributor

@torinmr torinmr commented Jan 9, 2024

The old implementation converted each byte to a hexadecimal number separately, meaning that leading zeros in a byte would be dropped even in the middle of a group.

For example, the address 2620:149:110b:470e:0:0:0:e1a was incorrectly
rendered as 2620:149:11b:47e:0:0:0:e1a, with zeros dropped
in the 3rd and 4th octets.

I figured the most robust fix would be to use inet_ntop to format the address, which produces a correctly abbreviated address like 2620:149:110b:470e::e1a. This is the approach used in DNSResolver_c-ares.swift.

The old implementation converted each byte to a hexadecimal number
separately, meaning that leading zeros in a byte would be dropped
even in the middle of a group.

For example, the address 2620:149:110b:470e:0:0:0:e1a was incorrectly
rendered as              2620:149:11b:47e:0:0:0:e1a, with zeros dropped
in the 3rd and 4th octets.

I figured the most robust fix would be to use inet_ntop to format the
address, which produces a correctly abbreviated address like
2620:149:110b:470e::e1a. This is the approach used in DNSResolver_c-ares.swift.
@yim-lee
Copy link
Member

yim-lee commented Jan 9, 2024

@swift-server-bot add to allowlist

@torinmr
Copy link
Contributor Author

torinmr commented Jan 9, 2024

Hmmm, apparently ./scripts/soundness.sh exits without printing anything if you don't have swiftformat installed. I fixed the formatting issues and added a patch to soundness.sh to print an informative error message in this case, let me know if it looks good to you.

Copy link
Member

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

Thanks @torinmr!

@yim-lee yim-lee merged commit 95fda89 into apple:main Jan 9, 2024
7 checks passed
@torinmr
Copy link
Contributor Author

torinmr commented Jan 9, 2024

Thanks for the review Yim! How does merging work? Looks like I don't have permission to trigger the merge myself.

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