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

Curl_resolver_init is not thread-safe #13065

Closed
nickitat opened this issue Mar 6, 2024 · 7 comments
Closed

Curl_resolver_init is not thread-safe #13065

nickitat opened this issue Mar 6, 2024 · 7 comments

Comments

@nickitat
Copy link

nickitat commented Mar 6, 2024

I did this

Hi!

We start getting sanitiser reports like this in our CI.

I assume that this code is supposed to be thread-safe, please correct me if I'm wrong.

If so, seems like the problematic code was introduced recently here:

4224d6e

so static variable ares_ver could be simultaneously read and modified by two threads.
it seems enough to replace l. 176 with

static int ares_ver = get_ares_version();

get_ares_version() will do the same call, but now compiler will guarantee single initialisation (btw, I'm not a C expert, do we have this guarantee in C?).

I expected the following

no sanitiser complains

curl/libcurl version

curl 8.6.0+

operating system

linux

jay added a commit to jay/curl that referenced this issue Mar 6, 2024
- Don't use a static variable to store the ares version.

Prior to this change several threads could write the same data to a
static int variable at the same time. Though in practice it's not a
problem ThreadSanitizer may warn.

Reported-by: Nikita Taranov

Fixes curl#13065
Closes #xxxxx
@jay
Copy link
Member

jay commented Mar 6, 2024

I don't see get_ares_version in our source or the c-ares api. See #13066 for proposed fix

jay pushed a commit to jay/curl that referenced this issue Mar 6, 2024
- Store the c-ares version during global init.

Prior to this change several threads could write the same data to a
static int variable at the same time. Though in practice it's not a
problem ThreadSanitizer may warn.

Reported-by: Nikita Taranov

Fixes curl#13065
Closes #xxxxx
@bagder
Copy link
Member

bagder commented Mar 7, 2024

I'm pretty sure this is still thread safe, even if we should make it done nicer.

@bagder bagder closed this as completed in 835e4cb Mar 7, 2024
@nickitat
Copy link
Author

nickitat commented Mar 7, 2024

thank you, guys
when could we expect the next release?

@bagder
Copy link
Member

bagder commented Mar 7, 2024

The next release ships on March 27, 2024 and will become version 8.7.0. The pending release notes for the coming release are always available on this URL: https://curl.se/dev/release-notes.html

@nickitat
Copy link
Author

nickitat commented Mar 7, 2024

I'd also love to get your advice on how we should proceed on our end. is it safe to use master version instead of the last release? to get the fix earlier. or maybe we could back-port the fix to the last release branch?

@jay
Copy link
Member

jay commented Mar 7, 2024

is it safe to use master version instead of the last release? to get the fix earlier. or maybe we could back-port the fix to the last release branch?

Do not use master in production. master is our development branch. I suggest use git to cherry pick the fixes you need.

@bagder
Copy link
Member

bagder commented Mar 7, 2024

is it safe to use master version instead of the last release?

I believe sticking to the previous version is fine until the next release ships. I think this not thread-safe warning was a warning and it should be and was fixed, but the effects of the problem are totally harmless.

SmitaRKulkarni pushed a commit to ClickHouse/curl that referenced this issue Mar 12, 2024
- Store the c-ares version during global init.

Prior to this change several threads could write the same data to a
static int variable at the same time. Though in practice it's not a
problem ThreadSanitizer may warn.

Reported-by: Nikita Taranov
Assisted-by: Jay Satiro

Fixes curl#13065
Closes curl#13000
SmitaRKulkarni pushed a commit to ClickHouse/curl that referenced this issue Mar 12, 2024
- Store the c-ares version during global init.

Prior to this change several threads could write the same data to a
static int variable at the same time. Though in practice it's not a
problem ThreadSanitizer may warn.

Reported-by: Nikita Taranov
Assisted-by: Jay Satiro

Fixes curl#13065
Closes curl#13000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants