quickjs Use closefrom if available

Replaces https://github.com/bellard/quickjs/pull/209

use closefrom if supported
fallback to the closing loop otherwise
compute fd_max using /proc/{pid}/fd on linux, if closefrom is not supported (ex: musl)

Feb 14 '24 20:02 nicolas-duteil-nova

Since closefrom() is supported in all recent systems, I don't see the advantage in having a complicated fallback using /proc. So I suggest to remove the complicated fallback and just use the loop as fallback.

Feb 15 '24 09:02 bellard

Since closefrom() is supported in all recent systems, I don't see the advantage in having a complicated fallback using /proc. So I suggest to remove the complicated fallback and just use the loop as fallback.

closefrom() is not supported in Musl libc for example. The fallback is mostly to deal with cases where closefrom is not supported and where relying on sysconf(_SC_OPEN_MAX); would impact the performances when _SC_OPEN_MAX is very high

Feb 15 '24 11:02 nicolas-duteil-nova

What about the last version which uses a separate function to compute the max number of file descriptors and close any descriptor >3 under linux @bellard ?

Feb 21 '24 12:02 nicolas-duteil-nova

What about the last version which uses a separate function to compute the max number of file descriptors and close any descriptor >3 under linux @bellard ?

We are still considering this patch.

closefrom should be used when available, including on linux with the musl C library, but we don't have a reliable way to test this special case with adding an extra configuration flag, which we like to avoid.
the loop could be used as a fallback if sysconf(_SC_OPEN_MAX) is large, but not to compute the largest handle, which might still be large in pathological cases, but to actually implement the closefrom emulation as done in the other project, while preserving the directory handle. dirfd has been available for this purpose on linux for a long time and is POSIX-1.2008 compliant.

Do you have a simple reliable way to test for closefrom availability ?

Feb 21 '24 12:02 chqrlie

What about the last version which uses a separate function to compute the max number of file descriptors and close any descriptor >3 under linux @bellard ?

We are still considering this patch.

closefrom should be used when available, including on linux with the musl C library, but we don't have a reliable way to test this special case with adding an extra configuration flag, which we like to avoid.

the loop could be used as a fallback if sysconf(_SC_OPEN_MAX) is large, but not to compute the largest handle, which might still be large in pathological cases, but to actually implement the closefrom emulation as done in the other project, while preserving the directory handle. dirfd has been available for this purpose on linux for a long time and is POSIX-1.2008 compliant.

Do you have a simple reliable way to test for closefrom availability ?

What if we conditionally define a HAVE_CLOSEFROM variable in the Makefile, after checking that closefrom is supported (by compiling a simplr file) ? (https://github.com/bellard/quickjs/pull/239/commits/f58fe3211bbc0c5b95f92af9800808a98c3c4e42)

Feb 21 '24 14:02 nicolas-duteil-nova

Interesting approach with a Makefile test run on the fly.

I would probably create a subdirectory compat for such source files to avoid clutter in the main directory. Recompiling this on every invocation including make clean is also a bit sloppy, but not necessarily a no-go.

Feb 21 '24 14:02 chqrlie

Interesting approach with a Makefile test run on the fly.

I would probably create a subdirectory compat for such source files to avoid clutter in the main directory. Recompiling this on every invocation including make clean is also a bit sloppy, but not necessarily a no-go.

I first try to use a Makefile target to do the compilation but I don't think it's possible to define/update a global variable from within a target. Anyway, compiling test-closefrom.c is fast enough to no be a pb imho.

I moved test-closefrom.c to compat directory and replaced the OS-based support detection with HAVE_CLOSEFROM

Feb 21 '24 15:02 nicolas-duteil-nova

There is still a problem: you should avoid closing the handle for the directory structure:

                        if (fd > 2 && fd != dirfd(dir)) {
                            close(fd);
                        }

Mar 03 '24 14:03 chqrlie

There is still a problem: you should avoid closing the handle for the directory structure:
                        if (fd > 2 && fd != dirfd(dir)) {
                            close(fd);
                        }

Right, it's better like that

Mar 03 '24 16:03 nicolas-duteil-nova

@chqrlie : can you approve the workflow ?

Mar 04 '24 11:03 nicolas-duteil-nova

Can we merge this one or does it need extra refactoring ? /cc @chqrlie

Mar 05 '24 15:03 nicolas-duteil-nova

@chqrlie : can we merge this one ?

May 09 '24 10:05 nicolas-duteil-nova

@chqrlie : can we merge this one ?

Sorry for the lack of responsiveness, a simpler approach seems advisable, see my proposed changes.

May 09 '24 13:05 chqrlie

@chqrlie : can we merge this one ?

Sorry for the lack of responsiveness, a simpler approach seems advisable, see my proposed changes.

I completely agree with the redirection to /dev/null but not sure about the rest. Overall, this PR is an improvement of current behaviour where we loop through all possible fds (which is inefficient). This PR uses closefrom when it is available and mitigate the loop problem when closefrom is not supported (which is only on alpine afaik).

What would be the adverse side effect of merging this PR @chqrlie ?

May 09 '24 17:05 nicolas-duteil-nova

What would be the adverse side effect of merging this PR @chqrlie ?

Well it adds lots of extra code as a partial solution for very specific case that is not so useful. Fabrice and I try and avoid such additions that may require maintenance and non trivial proofreading. The missing test on (fd != dirfd(dir)) is a good example of how subtle such things can become. Parsing the /proc/ subsystem is a good idea, but not worth the effort in this particular case IMHO.

May 09 '24 18:05 chqrlie

What would be the adverse side effect of merging this PR @chqrlie ?

Well it adds lots of extra code as a partial solution for very specific case that is not so useful. Fabrice and I try and avoid such additions that may require maintenance and non trivial proofreading. The missing test on (fd != dirfd(dir)) is a good example of how subtle such things can become. Parsing the /proc/ subsystem is a good idea, but not worth the effort in this particular case IMHO.

For the record, initial PR had no such pb :grin:

May 09 '24 19:05 nicolas-duteil-nova

For the record, initial PR had no such pb 😁

Indeed it did not, the loop was used to determine the largest handle in use, then the directory handle was closed and the loop run from 3 to that handle.

I'm sorry to see you close this PR, I posted a new one using the simplistic approach I suggested, that should fix the problem with alpine linux. Thank you for your interest in QuickJS.

May 09 '24 21:05 chqrlie

For the record, initial PR had no such pb 😁

Indeed it did not, the loop was used to determine the largest handle in use, then the directory handle was closed and the loop run from 3 to that handle.

I'm sorry to see you close this PR, I posted a new one using the simplistic approach I suggested, that should fix the problem with alpine linux. Thank you for your interest in QuickJS.

No worries @chqrlie. I was just under the impression that we were stalled, aguing over details. Glad your MR was merged (although I think that detecting max fd using /proc would have also handled the case where the number of fd is > 1024). Anyway, we all want to make quickjs a better tool, which is what matters in the end. Thanks again

May 10 '24 15:05 nicolas-duteil-nova