Use bsd_closefrom.c from openssh-portable to fix #189 #255
Conversation
|
Note: this isn't linking against musl libc at the moment, I probably need to get the macros in order... |
|
@thomasjm, what is the status of this? Does this now work against One thing that I am admittedly worried about here is the potential for C symbol name clashes. In particular, |
|
I ended up going with a workaround where I just lowered the max file descriptors in my Kubernetes environments, so this PR isn't quite done. IIRC I was still fumbling around with CMake options and having trouble getting this to build with I can help push it over the finish line if there's interest. |
|
It does seem like a nice improvement; it would be great to get it upstream. |
|
Looks good save one small question. |
cbits/posix/runProcess.c
Outdated
| closefrom_excluding(int lowfd, int excludingFd) { | ||
| // Try using the close_range syscall, provided in Linux kernel >= 5.9. | ||
| // We do this directly because not all C libs provide a wrapper (like musl) | ||
| long ret = syscall(CLOSE_RANGE_SYSCALL_NUMBER, lowfd, excludingFd - 1, 0); |
There was a problem hiding this comment.
This line is a little dodgy due to the direct syscall. It should be good on Linux kernels because Linux has very strong guarantees about syscall stability. For other POSIX platforms we probably need to gate this behind a macro. Another option would be to not even try using the close_range syscall, and just loop from lowFd to excludingFd, assuming that excludingFd will be reasonably small.
FWIW there's a possibility that close_range will be added to musl in the future: https://inbox.vuxu.org/musl/[email protected]/T/#u
It would help me move this along if I had an easy way to test musl builds. I have a somewhat convoluted method using Haskell.nix, but do you happen to know a good way @bgamari ?
There was a problem hiding this comment.
I would likely just use GHC's CI infrastructure. That is, open a draft MR against ghc/ghc bumping the process submodule to your branch. This will get tested against Alpine 3.12.
|
Do let me know when you would like a review on this, @thomasjm . |
See: openssh/openssh-portable@10b899a1 openssh/openssh-portable@715c892f This brings our copy up to match openssh-portable as of Dec 22, 2021
|
Thanks @bgamari, I think it's just about ready for review!
I've done this in the latest commit, does that look like what you had in mind? The GHC pipeline is passing at this point. I think what we could still use is some test(s) that file descriptors actually get closed properly in I'm also wondering what happens on *BSD or other platforms where the syscall could potentially differ. Poking around in the FreeBSD source it looks like the arguments match Linux. I'm still scared of the direct syscall, although I really want this to work on static Musl builds. FWIW, I just pushed a commit to bring |
I'm trying to fix #189.
Closing file descriptors in a reliable and portable way is not simple. The best summary I found of the different approaches available is in this StackOverflow answer.
Modern versions of
glibc(>= 2.34) and/or the Linux kernel (>= 5.9) have added support for theclosefromandclose_rangefunctions, which allow us to performantly do what we want. A good solution will use these functions when available, and otherwise use fallbacks. Putting together the fallbacks is a little involved.In this PR I added the BSD-licensed
bsd_closefrom.cfile from https://github.com/openssh/openssh-portable/ and use it to implement the FD-closing logic from this library.I haven't tested this yet, I'm putting it up to see how it does in CI and solicit feedback on the approach. Thanks!