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

Support operating systems with only getaddrinfo(3) #15475

Closed
wants to merge 1 commit into from

Conversation

sortie
Copy link
Contributor

@sortie sortie commented Nov 1, 2024

The gethostbyname(3) family was removed in POSIX-1.2008 in favor of getaddrinfo(3) introduced in POSIX-1.2001. Modern POSIX systems such as Sortix does not have gethostbyname nor the related definitions and structures.

curl already only uses getaddrinfo(3) if available and thread safe, although there is mild breakage if the related gethostbyname definitions are missing.

This change attempts to fix that breakage:

Remove an unnecessary configure error if gethostbyname is missing since getaddrinfo is enough as a fallback.

Rewrite Curl_ip2addr to not use struct hostent as it no longer is standardized and create the struct Curl_addrinfo directly.

Only define the Curl_he2ai function on non-getaddrinfo systems where it is going to be used with struct hoestent.

Revoke the fallback logic for when it's unknown whether getaddrinfo is thread safe. It doesn't appear to make any sense since h_errno is unrelated to getaddrinfo. The logic prevents new POSIX.1-2024 systems from passing the thread safety test since h_errno does not exist anymore and POSIX already requires getaddrinfo to be thread safe. There's already a denylist in place for operating systems with known buggy implementations.

--

I'm not sure what's up with those pragmas but I removed them, since I rewrote the code and the new implementation is quite clean and parts comes from above, where they were not needed.

Curl_he2ai may still be called if the lib/asyn-ares.c happens to be compiled in on a system with a thread-safe getaddrinfo. I'm unclear what ares even is and whether this would happen. Let me know if the logic needs to be amended or cleaned up.

The gethostbyname(3) family was removed in POSIX-1.2008 in favor of
getaddrinfo(3) introduced in POSIX-1.2001. Modern POSIX systems such as
Sortix does not have gethostbyname nor the related definitions and
structures.

curl already only uses getaddrinfo(3) if available and thread safe,
although there is mild breakage if the related gethostbyname definitions
are missing.

This change attempts to fix that breakage:

Remove an unnecessary configure error if gethostbyname is missing since
getaddrinfo is enough as a fallback.

Rewrite Curl_ip2addr to not use struct hostent as it no longer is
standardized and create the struct Curl_addrinfo directly.

Only define the Curl_he2ai function on non-getaddrinfo systems where it
is going to be used with struct hoestent.

Revoke the fallback logic for when it's unknown whether getaddrinfo is
thread safe. It doesn't appear to make any sense since h_errno is
unrelated to getaddrinfo. The logic prevents new POSIX.1-2024 systems
from passing the thread safety test since h_errno does not exist anymore
and POSIX already requires getaddrinfo to be thread safe. There's
already a denylist in place for operating systems with known buggy
implementations.
@bagder
Copy link
Member

bagder commented Nov 2, 2024

unclear what ares even is and whether this would happen

c-ares is a 3rd party asynchronous name resolver library. curl can optionally get built to use that instead of the "normal" resolver functions, which then will not call getaddrinfo() at all.

We have CI jobs doing this and since all the CI ran green, it is a good indication your PR does not break that functionality.

@bagder bagder closed this in 78c3172 Nov 2, 2024
@bagder
Copy link
Member

bagder commented Nov 2, 2024

Thanks!

@sortie
Copy link
Contributor Author

sortie commented Nov 2, 2024

Woohoo thanks Daniel! Was fun to contribute to curl!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants