Darwin: Only set FD_CLOEXEC when creating a socket FD

[carlhoerberg]

Not when an existing FD is passed to Socket.new

[carlhoerberg]

Follow up on https://github.com/crystal-lang/crystal/pull/14632#pullrequestreview-2080836656

[ysbaddaden]

I notice some issues with this change: when we create the socket the @handle ivar isn't set and calling #fcntl will fail (see CI) but we also set CLOEXEC for plain fd (e.g. Socket.new(fd, ...) as called from UNIXSocket.pair or Socket#accept? for example.

[straight-shoota]

when we create the socket the @handle ivar isn't set and calling #fcntl will fail (see CI)

Yeah I think we need to use LibC.fcntl(fd, ...) like before to fix the current failure. The file descriptor is not assigned yet, so we cannot use #fcntl.

we also set CLOEXEC for plain fd (e.g. Socket.new(fd, ...) as called from UNIXSocket.pair or Socket#accept? for example.

Yes, that's the current behaviour. But I believe it's wrong. We probably shouldn't autoclose a file descriptor that's coming from outside the process. Only close what we opened ourselves. That means all uses in stdlib like Socket#accept? need to be updated to explicitly set close on exec.

However, I'm wondering if this isn't too bad a silent breaking change in behaviour... 🤔

[ysbaddaden]

I agree: it may not be the best API, but security-wise it doesn't sound like a bad practice to always close file descriptors on exec... and maybe always calling fcntl ain't a bad idea instead of setting O_CLOEXEC after all? If you need it to not be closed on exec, you can always call fcntl to disable the flag.

[straight-shoota]

Yeah, I suppose we should also setup specs for those methods.

@carlhoerberg If you want to go on, please continue. But we'll be happy to take over these final steps if you prefer. Just say the word.

[carlhoerberg]

Go for it! :)