Use bsd_closefrom.c from openssh-portable to fix #189

[thomasjm]

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 the closefrom and close_range functions, 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.c file 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!

[thomasjm]

Note: this isn't linking against musl libc at the moment, I probably need to get the macros in order...

[bgamari]

@thomasjm, what is the status of this? Does this now work against musl? The implementation looks good on a cursory look although this does need to be rebased so I have held off on a thorough review.

One thing that I am admittedly worried about here is the potential for C symbol name clashes. In particular, process's cbits exposes many symbols with extremely generic names; this is certainly not a new problem but the fact that we now use code from OpenSSH increases the chance that things will break in the wild. Ideally, Haskell libraries would prefix all exported C symbols with a semi-unique prefix (e.g. hs_process).

[thomasjm]

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 musl.

I can help push it over the finish line if there's interest.

[bgamari]

It does seem like a nice improvement; it would be great to get it upstream.

[bgamari]

Looks good save one small question.

[bgamari]

Do let me know when you would like a review on this, @thomasjm .

[thomasjm]

Thanks @bgamari, I think it's just about ready for review!

Ideally, Haskell libraries would prefix all exported C symbols with a semi-unique prefix (e.g. hs_process).

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 fork_exec.c.

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 bsd_closefrom.c up to date with a couple upstream changes I noticed we were missing. One of those changes involves an actual call close_range, gated on HAVE_CLOSE_RANGE. We should probably be consistent about this: if we decide a close_range syscall is okay, we should replace that call in bsd_closefrom.c also.