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: Make socket callback during cleanup into no-op #3307

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Sep 18, 2024

Because curl_multi_cleanup may invoke callbacks, we effectively have
some circular references going on here. See discussion in

curl/curl#14860

Basically what we do is the socket callback libcurl may invoke into a no-op when
we detect we're finalizing. The data structures are owned by this object and
not by the callbacks, and will be destroyed below. Note that
e.g. g_hash_table_unref() may itself invoke callbacks, which is where
some data is cleaned up.

Signed-off-by: Colin Walters [email protected]

Closes: #3299


Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

Because curl_multi_cleanup may invoke callbacks, we effectively have
some circular references going on here. See discussion in

curl/curl#14860

Basically what we do is the socket callback libcurl may invoke into a no-op when
we detect we're finalizing. The data structures are owned by this object and
not by the callbacks, and will be destroyed below. Note that
e.g. g_hash_table_unref() may itself invoke callbacks, which is where
some data is cleaned up.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters force-pushed the curl-reorder-teardown branch from 34e4f1a to 4d755a8 Compare September 18, 2024 21:42
@cgwalters cgwalters changed the title curl: Only free multi handle as last action curl: Make socket callback during cleanup into no-op Sep 18, 2024
@cgwalters
Copy link
Member Author

cgwalters commented Sep 18, 2024

For people who are distributing ostree: in my opinion this patch is quite safe to cherry pick. The affected code here is during the teardown path and we're just making a called function into a no-op, so realistically the worst case is memory leaks. But for leaks: ASAN is still happy with this change, and from my auditing of the code again we're always relying on our own teardown, not on the libcurl socket callback.

@cgwalters cgwalters enabled auto-merge September 18, 2024 22:39
@cgwalters cgwalters disabled auto-merge September 18, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flatpak update produces ostree error with curl 8.10.0 (+ patch)
4 participants