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

lib: sync guard for Curl_getaddrinfo_ex() definition and use #9734

Closed
wants to merge 2 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 14, 2022

Curl_getaddrinfo_ex() gets defined with HAVE_GETADDRINFO set. But, hostip4.c used it with HAVE_GETADDRINFO_THREADSAFE set alone. It meant a build with the latter, but without the former flag could result in calling this function but not defining it, and failing to link.

Patch this by adding an extra check for HAVE_GETATTRINFO around the call.

Before this patch, build systems prevented this condition. Now they don't need to, so delete the build-system-specific logic from CMake.

Follow-up to 67d8862

Closes #xxxx

@vszakats
Copy link
Member Author

vszakats commented Oct 14, 2022

Ref: #9727 (comment)

`Curl_getaddrinfo_ex()` gets _defined_ with `HAVE_GETADDRINFO` set.
But, `hostip4.c` _used_ it with `HAVE_GETADDRINFO_THREADSAFE` set alone.
It meant a build with the latter, but without the former flag could
result in calling this function but not defining it, and failing to
link.

Patch this by adding an extra check for `HAVE_GETATTRINFO` around
the call.

Before this patch, build systems prevented this condition. Now they
don't need to.

Follow-up to 67d8862

Closes #xxxx
@@ -1079,9 +1079,6 @@ check_symbol_exists(_strtoi64 "${CURL_INCLUDES}" HAVE__STRTOI64)
check_symbol_exists(strerror_r "${CURL_INCLUDES}" HAVE_STRERROR_R)
check_symbol_exists(siginterrupt "${CURL_INCLUDES}" HAVE_SIGINTERRUPT)
check_symbol_exists(getaddrinfo "${CURL_INCLUDES}" HAVE_GETADDRINFO)
if(NOT HAVE_GETADDRINFO)
set(HAVE_GETADDRINFO_THREADSAFE OFF)
endif()
Copy link
Member

@jay jay Oct 26, 2022

Choose a reason for hiding this comment

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

wouldn't it make sense to leave this

Copy link
Member Author

@vszakats vszakats Oct 26, 2022

Choose a reason for hiding this comment

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

It wouldn't hurt, but why guard against this twice?

Also without it, our CI wouldn't be really verifying this PR.

Copy link
Member

@jay jay Oct 26, 2022

Choose a reason for hiding this comment

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

I don't see where else it is checked. What I am saying is it wouldn't make sense that HAVE_GETADDRINFO_THREADSAFE is defined when not HAVE_GETADDRINFO.

Copy link
Member Author

@vszakats vszakats Oct 26, 2022

Choose a reason for hiding this comment

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

The root problem was that hostip4.c assumed that if HAVE_GETADDRINFO_THREADSAFE was set, also HAVE_GETADDRINFO was set. This was not always true, so I added this condition to CMake. (autotools was setting them in pair already). This still didn't prevent a manual/accidental misconfiguration. Hence this PR, where I address the root cause in hostip4.c, making sure to check that both are set, there. This also makes the manual CMake condition redundant.

[ You may ask, how could CMake set HAVE_GETADDRINFO_THREADSAFE if there was no getaddrinfo() present. The reason for that is that HAVE_GETADDRINFO_THREADSAFE is force-set to 1 on Windows, while the latter is properly auto-detected! ]

Copy link
Member

@jay jay Oct 27, 2022

Choose a reason for hiding this comment

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

Ok. IMO it wouldn't hurt to leave it there.

Copy link
Member Author

@vszakats vszakats Oct 27, 2022

Choose a reason for hiding this comment

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

Fine with me, I'll put it back there.

Copy link
Member Author

@vszakats vszakats Oct 27, 2022

Choose a reason for hiding this comment

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

@jay: Rephrasing the Windows logic as this:

if(WIN32)
  set(HAVE_GETADDRINFO_THREADSAFE ${HAVE_GETADDRINFO})
endif()

would IMO express our intent much cleaner and self contained in these three lines, with identical results. It would also allow dropping the related line from WindowsCache.cmake.

Meaning: When we have getaddrinfo(), it is always threadsafe (on Windows).

What do you think?

Copy link
Member

@jay jay Oct 27, 2022

Choose a reason for hiding this comment

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

Works for me

jay
jay approved these changes Oct 27, 2022
Simplify 67d8862, by setting
`HAVE_GETADDRINFO_THREADSAFE` to the detection result of
`HAVE_GETADDRINFO` on Windows. This expresses this intent clearer than
the previous patch and keeps this logic in a single block of code:

On Windows when we have `getaddrinfo()` it is always threadsafe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants