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

Don't pass socket file descriptors to subprocesses on Unix (SOCK_CLOEXEC) #14632

Merged
merged 3 commits into from
May 29, 2024

Conversation

carlhoerberg
Copy link
Contributor

The comment 8 lines down suggests that this has been lost at some point.

Fixes #14630

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Searching the history, I think we never had it. Let's fix that!

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

This seems like the right thing to do. It was probably just missed in the original implementation.

suggestion: Add comments with cross-references between the different implementations in create_handle and initialize_handle.
I'm also wondering if the fcntl part shouldn't be moved to create_handle, but that's a different issue.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking labels May 27, 2024
@straight-shoota
Copy link
Member

straight-shoota commented May 27, 2024

suggestion: Add a simple spec to ensure this doesn't get lost somewhere. A simple check for close_on_exec? should do:

it "socket closes on exec by default" do
  socket = Socket.new(Socket::Family::INET, Socket::Type::STREAM, Socket::Protocol::TCP)
  socket.close_on_exec?.should be_true
end

@ysbaddaden
Copy link
Contributor

I disabled the spec for windows. This is an UNIX only consideration.

@straight-shoota
Copy link
Member

We still need to take a look at the Windows stuff. I've created #14636 to track that.

@straight-shoota straight-shoota added this to the 1.13.0 milestone May 28, 2024
@straight-shoota straight-shoota changed the title Use SOCK_CLOEXEC to not inherit sockets to subprocesses Don't pass socket file descriptors to subprocesses on Unix (SOCK_CLOEXEC) May 28, 2024
@straight-shoota straight-shoota merged commit ca7aae5 into crystal-lang:master May 29, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sockets are inherited by subprocesses
3 participants