Improve file descriptor closing loops

[Dim-P]

Summary

Fixes #14177 , fixes #14062 .

There are 3 places in the agent where we either try to close all open file descriptors or mark them to be closed by the exec() stage of posix_spawn(). This is done in a bruteforce way, by looping through all FD IDs up to _SC_OPEN_MAX.

While this is a valid approach, it creates issues on platforms that (incorrectly) set the maximum FD limit to infinity (in practice, it can be as high as 1073741816).

This PR instead of bruteforcing its way through all possible FD IDs, it reads /proc/self/fd (if available) to discover which FDs are actually open and closes (or marks to be closed) only them (unless STDIN, STDOUT and/or STDERR are to be excluded).

This should also speed up slightly the agent initialisation time (by how much, it depends on the _SC_OPEN_MAX value) .

Test Plan

Test on a system with a very high ulimit -n such as 1073741816. The agent should start up in reasonable time instead of taking >10min and consuming 100% CPU in the meantime.

TODO: Revert #14178

[ilyam8]

TODO: Revert #14178

We can't do that until a new stable version is released.

[Dim-P]

TODO: Revert #14178

We can't do that until a new stable version is released.

That's correct, that's why I didn't include it in this PR, but added it as TODO so that we don't forget about it.

[ktsaou]

Various operating systems have different methods for massively closing file descriptors.

1. Linux has close_range().
2. BSDs have closefrom().

I think the /proc/PID/fd traversal is slow and should be a fallback when the above are not available.

So, I think the process should be like this:

1. Use close_range() when it is available.
2. If not available, use closefrom() when it is available.
3. If not available, use /proc/PID/fd traversal when it is available.
4. If not available, use sysconf() like we currently do.

To use close_range() or closefrom() we need to test for them with automake.

By adding this line to configure.ac (I think between lines 254 and 255):

AC_CHECK_FUNCS([close_range closefrom])

We are going to have in config.h the following:

/* Define to 1 if you have the `closefrom' function. */
#define HAVE_CLOSEFROM 1

/* Define to 1 if you have the `close_range' function. */
#define HAVE_CLOSE_RANGE 1

After this, we can do:

#if defined(HAVE_CLOSE_RANGE)
   // use close_range() to close the open file descriptors
#elif defined(HAVE_CLOSEFROM)
   // use closefrom() to close the open file descriptors
#else
   // try to use /proc/PID/fd to close the open file descriptors
   // if the above fails, try to use getrlimit()
   // if the above fails, fallback to sysconf(_SC_OPEN_MAX)
#endif

[ktsaou]

I see in the code that there are 2 possible actions: close and mark with FD_CLOEXEC.

I think the later in popene_internal() is an attempt to use the FD_CLOEXEC flag to let posix_spawn() of the file descriptors to keep open and the ones to close.

Generally speaking, doing this loop at popene_internal() is kind of a waste. All the descriptors should be marked with FD_CLOEXEC when they are first opened, not before forking.

If we can't mark them like that when they are opened, I suggest to drop the marking entirely and once to fork is done by posix_spawn() to close them while running as the child process.

This means that we are going to need only one action for for_each_open_fd(), to close the open files, so we can also rename it to close_all_open_fds() given the exception list.

[ktsaou]

@Dim-P please also use enum not just int, to have better understanding of the type of values accepted. We make a lot of changes to the code base and having enum improves maintainability tremendously.

[Dim-P]

Thank you for the comments @ktsaou .

1. close_range() is a good call, I've used it as the first candidate to close the fds. It exists on FreeBSD as well.
2. re. FD_CLOEXEC, I had a look at the code and you are right, it can probably be removed altogether, but this required more changes than just the plain fixes this PR is meant to do, so it can be done in a future PR. Maybe it's simpler than I thought, but need to check all netdata_popen() occurrences.
3. I didn't use closefrom() since it doesn't support FD_CLOEXEC, but I don't think it was going to be of much use anyway.
4. I replaced int with enum where applicable.

Tested on Ubuntu 22.04, Fedora 36, FreeBSD 13.1, Alpine 3.17.

[vkalintiris]

FWIW, I think we are overthinking this. The original reason to close open FDs was lxc-attach, see #1775.

There are 3 places in the agent where we either try to close all open file descriptors or mark them to be closed by the exec() stage of posix_spawn().

Skimming through the code, this is (or should be) actually 2 places:

1. When the agent starts,
2. When the agent spawns the spawn server,
3. Inside libnetdata's popen.

We should coalesce 1 and 2. That is, we should spawn the spawn server only after closing all open FDs.

This is done in a bruteforce way, by looping through all FD IDs up to _SC_OPEN_MAX.

The original issue was lxc-attach leaving an open pseudoterminal device open. I'd bet that closing just a small, fixed amount of open FDs (let's say 64-128) would be more than safe. We can handle then the opening of our own FDs with the proper flag to close on exec.

[thiagoftsm]

As we discussed internally, I am approving this PR expecting that suggestions from @vkalintiris will be address in another PR. LGTM!

[Dim-P]

Thanks guys, @vkalintiris I will merge this now to fix the issue before the upcoming stable release and we can make a new PR to improve the code using your suggestions.

[ilyam8]

TODO: Revert #14178

@Dim-P it is time now!