Skip to content

share: concurrency handling, easy updates#20870

Closed
icing wants to merge 11 commits intocurl:masterfrom
icing:share-share
Closed

share: concurrency handling, easy updates#20870
icing wants to merge 11 commits intocurl:masterfrom
icing:share-share

Conversation

@icing
Copy link
Copy Markdown
Contributor

@icing icing commented Mar 9, 2026

curl share:

Replace the volatile int dirty with a reference counter protected by a mutex when available.

Solve the problem of when to call application's lock function by adding a volatile flag that indicates a share has been added to easy handles in its lifetime. That flag ever goes from FALSE to TRUE, so volatile might work (in the absence of a mutex).

(The problem is that the lock/unlock functions need 2-3 curl_share_setopt() invocations to become usable and there is no way of telling if the third will ever happen. Calling the lock function before the 3rd setopt may crash the application.)

When removing a share from an easy handle (or replacing it with another share), detach the easy connection on a share with a connection pool.

configure/cmake:

Disentangle mutex/threads availability from threaded resolver. We want to use mutex also when other resolvers are configured. This means:

  • USE_THREADS_POSIX and USE_THREADS_WIN32 gone
  • replaced with HAVE_THREADS_POSIX and _WIN32
  • introduce USE_RESOLV_THREADED and USE_RESOLV_ARES
  • introduce curl_setup.h defines
    - USE_MUTEX when pthread/win32 mutex should be used
    - USE_THREADS when threads are used
  • remove CURLRES_ARES and CURLRES_THREADED

@icing icing requested a review from bagder March 9, 2026 15:16
@bagder
Copy link
Copy Markdown
Member

bagder commented Mar 9, 2026

@aisle-analyzer augment review

@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot bot commented Mar 9, 2026

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

# Severity Title
1 🔵 Low Race condition in curl_share_cleanup can free share during first-time link (UAF)

See details in the comment below.


Analyzed PR: #20870 at commit 9ad5222

Last updated on: 2026-03-09T22:34:54Z

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 9, 2026

🤖 Augment PR Summary

Summary: This PR reworks share-handle concurrency tracking to avoid relying on a volatile “dirty” counter and to make share attachment/detachment from easy handles safer.

Changes:

  • Replaces the former volatile dirty usage tracking with a ref_count reference counter, protected by an internal mutex when thread support is available.
  • Adds helper routines to destroy shares, increment/decrement references, and manage the “has been shared” lifetime flag used to decide when app-provided lock callbacks are safe to invoke.
  • Updates curl_share_setopt() to reject option changes while the share is in use via share_in_use().
  • Refactors CURLOPT_SHARE handling into Curl_share_easy_link/unlink() to centralize share attach/detach logic.
  • On share removal/replacement, detaches the easy handle’s connection when using a shared connection pool and unlinks DNS/cookie/HSTS/PSL pointers as applicable.
  • Updates Curl_close() to unlink the easy handle from its share through the new helper.

Technical Notes: The new has_been_shared flag aims to prevent invoking application lock callbacks too early (before the lock/unlock/userdata trio is fully configured) when the share has never been used by an easy handle.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@curl curl deleted a comment from testclutch Mar 10, 2026
@icing icing added the feature-window A merge of this requires an open feature window label Mar 10, 2026
@icing icing requested a review from vszakats March 10, 2026 12:18
@icing
Copy link
Copy Markdown
Contributor Author

icing commented Mar 10, 2026

@vszakats I change cmake files without really having a clue. It seems to work, but I'd like your sharp eyes on this. The changes were needed because the new code needs to use pthread/win32 mutex things, even when no threaded resolver is configured.

USE_THREADS_POSIX was really a bad name. I split that into HAVE_THREADS_POSIX and USE_RESOLV_THREADED.

@bagder
Copy link
Copy Markdown
Member

bagder commented Mar 21, 2026

Because I merged some of your older PRs first, this now needs some merge conflicts sorted out.

icing added 10 commits March 21, 2026 14:00
Replace the `volatile int dirty` with a reference counter
protected by a mutex when available.

Solve the problem of when to call application's lock function
by adding a volatile flag that indicates a share has been added
to easy handles in its lifetime. That flag ever goes from
FALSE to TRUE, so volatile might work (in the absence of a mutex).

(The problem is that the lock/unlock functions need 2-3
`curl_share_setopt()` invocations to become usable and there
is no way of telling if the third will ever happen. Calling
the lock function before the 3rd setopt may crash the
application.)

When removing a share from an easy handle (or replacing it with
another share), detach the easy connection on a share with a
connection pool.

When cleaning up a share, allow this even if it is still used in
easy handles. It will be destroyed when the reference count
drops to 0.
share: use `share_in_use` for checking cleanup deny

configure/cmake defines:

Disentangle mutex/threads availability from threaded resolver. We
want to use mutex also when other resolvers are configured. This
means:

- USE_THREADS_POSIX and USE_THREADS_WIN32 gone
- replaced with HAVE_THREADS_POSIX and HAVE_THREADS_WIN32
- introduce USE_RESOLV_THREADED and USE_RESOLV_ARES
- introduce curl_setup.h defines
  - USE_MUTEX when pthread/win32 mutex should be used
  - USE_THREADS when threads are used
- remove CURLRES_ARES and CURLRES_THREADED

Will probably break in CI for cmake builds, we'll see.
@icing
Copy link
Copy Markdown
Contributor Author

icing commented Mar 21, 2026

rebased

@bagder bagder closed this in 82009c4 Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmdline tool feature-window A merge of this requires an open feature window libcurl API tests

Development

Successfully merging this pull request may close these issues.

3 participants