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_LOCK_DATA_CONNECT can cause concurrent access to a connection on multiple threads #4544

Closed
c29reid opened this issue Oct 30, 2019 · 4 comments
Assignees
Labels

Comments

@c29reid
Copy link

@c29reid c29reid commented Oct 30, 2019

I did this

We've seen race conditions when using CURL_LOCK_DATA_CONNECT in libcurl where sometimes two different threads using two different easy handles ends up sharing the same connection pointer at the same time.
This causes crashes when both threads are doing work on the same connection pointer.

I added curl cpp code which stresses CURL_LOCK_DATA_CONNECT and should eventually trigger an ASAN error or crash with curl compiled using clang's address sanitizers.
It's not consistent how it fails since it's a threading issue. I've found that it's more consistent after adding a random sleep after the unlock here https://github.com/curl/curl/blob/master/lib/url.c#L1372.

Sample program which can reproduce this issue:
curl.txt

An example of ASAN output when this issue is hit with additional logging.
asan-output.txt

Notably three threads with different easy handles decide to reuse the 0x61b000fbd688 connection at the same time.

[thread: 825eb700] url.c:1370 chosen connection=0x61b000fbd688
[thread: 825eb700] reuse_conn(old_conn=0x61b000979188, conn=0x61b000fbd688)
[thread: 7bdde700] url.c:1370 chosen connection=0x61b000fbd688
[thread: 7bdde700][thread: 8a7ff700] Cur reuse_conn(old_conn=0x61b0012e1888, conn=0x61b000fbl.c:1370 chosen connecd688)
[thread: 877f9700] url_attach_connnection(data = 0x6230002c4d08,  conn = 0x61b000f1a388)
tion=0x61b000fbd688
[thread: 877f9700] reuse_conn(old_conn=0x61b000a94988, conn=0x61b000fbd688)
[thread: 825eb700] Curl_attach_connnection(data = 0x62300030e508,  conn = 0x61b000fbd688)
[thread: 7bdde700] Curl_attach_connnection(data = 0x6230000e3908,  conn = 0x61b000fbd688)
[thread: 877f9700] Curl_attach_connnection(data = 0x62300021cd08,  conn = 0x61b000fbd688)

curl/libcurl version

This issue is reproducible on libcurl master.

operating system

Ubuntu

@bagder bagder self-assigned this Oct 30, 2019
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Oct 30, 2019

I can reproduce and I have a patch in progress. It currently seem to fix the issue partly but not completely and I'm starting to get a clue regarding what's missing. It will probably take me a few more days to present a fix.

bagder added a commit that referenced this issue Nov 3, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Nov 3, 2019

This patch seems to have removed the crashes from running the trigger program...

bagder added a commit that referenced this issue Nov 4, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
@c29reid

This comment has been minimized.

Copy link
Author

@c29reid c29reid commented Nov 4, 2019

Awesome, it's looking much more stable with those changes. I haven't seen any ASAN failure or crash yet with the repro code stressing it.

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Nov 4, 2019

It is certainly a step in the right direction. I'll test the PR a bit more before I land it, since I got some suspicious-looking freebsd crashes first that now don't occur... I don't think I'll have the nerve to merge it before the Nov 6 release anyway!

bagder added a commit that referenced this issue Nov 6, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
bagder added a commit that referenced this issue Nov 8, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
bagder added a commit that referenced this issue Dec 2, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
bagder added a commit that referenced this issue Dec 3, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
bagder added a commit that referenced this issue Dec 9, 2019
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.

Reported-by: Christopher Reid
Fixes #4544
@bagder bagder closed this in ee263de Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.