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

Set FD_CLOEXEC on sockets created by curl #12439

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Conversation

MaxHearnden
Copy link
Contributor

Motivation

Curl creates sockets without setting FD_CLOEXEC/SOCK_CLOEXEC, this can cause connections to remain open forever when using commands like nix shell

This change sets the FD_CLOEXEC flag using a CURLOPT_SOCKOPTFUNCTION callback.

Context

This is implemented based on comments in curl/curl#2252


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Curl creates sockets without setting FD_CLOEXEC/SOCK_CLOEXEC, this can
cause connections to remain open forever when using commands like `nix
shell`

This change sets the FD_CLOEXEC flag using a CURLOPT_SOCKOPTFUNCTION
callback.
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Looks like a good solution to the problem.
I suppose we could use unix::closeExtraFDs() in more places, but that would break the possible use case of creating a nix run or nix shell subprocess with more than 3 fds.

@@ -300,6 +300,14 @@ struct curlFileTransfer : public FileTransfer
return ((TransferItem *) userp)->readCallback(buffer, size, nitems);
}

#if !defined(_WIN32) && LIBCURL_VERSION_NUM >= 0x071000
Copy link
Member

Choose a reason for hiding this comment

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

Is active 👍

@edolstra edolstra merged commit ca7e686 into NixOS:master Feb 12, 2025
12 checks passed
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.

3 participants