Fix gethostbyaddr for IPv6 arguments and make it domain-safe #11461
Conversation
OlivierNicole
changed the title
|
Unfortunately I am not able to test these changes properly, as IPv6 does not work on my setup… it is also unclear to me whether a test can be added to the test suite, given the uncertain nature of IPv6 support. |
|
Also a Changes entry is in order I think. |
OlivierNicole
force-pushed
the
fixgethost
branch
from
0f0ac10
to
c26c79d
Compare
|
Changes entry added. |
|
@dra27 do you want to take a second look? |
There was a problem hiding this comment.
Are you sure that IPv6 isn't working on your system - the code has definitely become domain-safe, but the IPv6 portion isn't yet correct. You should be able to do Unix.(gethostbyaddr (inet_addr_of_string "::1")). The correct flags (apart from an argument typo in the gethostbyaddr case!) were being passed, but the struct itself was wrong - it needs to be struct in6_addr for an IPv6 address, converted using GET_INET6_ADDR. I've put the version I just tested on my system in as review comments - this resolves both ::1 (localhost-ip6) and 2001:4860:4860::8888.
As far as I can tell, this has been wrong since the dawn of time?! 🙂 The IPv6 part of the fix should definitely go in the 4.14 branch as well.
Assuming all of the Jenkins hosts are IPv6-configured, checking that Unix.(gethostbyaddr (inet_addr_of_string "::1")) doesn't raise would be a good enough test, I think.
OlivierNicole
force-pushed
the
fixgethost
branch
2 times, most recently
from
5843d6f
to
bffb305
Compare
|
Thank you for the careful review! I confirm that your fixes allows me to resolve |
| let () = | ||
| let addr = Unix.inet_addr_of_string "127.0.0.1" in | ||
| ignore @@ Sys.opaque_identity Unix.gethostbyaddr addr; | ||
| let addr = Unix.inet_addr_of_string "::1" in | ||
| ignore @@ Sys.opaque_identity Unix.gethostbyaddr addr; |
There was a problem hiding this comment.
Why opaque_identity? Also, you could check parts of the result, e.g. the protocol family:
| let () = | |
| let addr = Unix.inet_addr_of_string "127.0.0.1" in | |
| ignore @@ Sys.opaque_identity Unix.gethostbyaddr addr; | |
| let addr = Unix.inet_addr_of_string "::1" in | |
| ignore @@ Sys.opaque_identity Unix.gethostbyaddr addr; | |
| let check a ty = | |
| let addr = Unix.inet_addr_of_string a in | |
| let host = Unix.gethostbyaddr addr in | |
| assert (host.Unix.h_addrtype = ty) | |
| let () = | |
| check "127.0.0.1" Unix.PF_INET; | |
| check "::1" Unix.PF_INET6; |
There was a problem hiding this comment.
Thank you, I integrated this suggestion.
Why opaque_identity?
I'm always a bit unsure about what might be optimized away.
caml_gethostbyaddr would always call gethostbyaddr with an IPv4 address type and structure, even when its argument is an IPv6 address. This commit also removes a file-local mutable variable for the sake of domain safety.
OlivierNicole
force-pushed
the
fixgethost
branch
from
bffb305
to
861b931
Compare
|
I'll merge and cherry-pick to 5.0 when CI is green. |
caml_gethostbyaddr would always call gethostbyaddr with an IPv4 address type and structure, even when its argument is an IPv6 address. This commit also removes a file-local mutable variable for the sake of domain safety. Added a test. (cherry picked from commit 4ec8c91)
The fix from ocaml#11461 did not handle the case where HAS_IPV6 is not defined.
The fix from #11461 did not handle the case where HAS_IPV6 is not defined.
caml_gethostbyaddralways callsgethostbyaddrwith address typeAF_INET, i.e., IPv4, even when its argument is an IPv6 address. This commit also fixes a domain safety bug by splitting a function in two rather than using a file-local mutable variable.