On Wed, 25 Aug 2021 at 15:15, Michael Hudson-Doyle <
michael.hudson@canonical.com> wrote:
> Hi, thanks for looking at this.
>
> On Wed, 25 Aug 2021 at 07:29, Adhemerval Zanella via Libc-alpha <
> libc-alpha@sourceware.org> wrote:
>
>> It ensures a continuous range of file descriptor and avoid hitting
>> the RLIMIT_NOFILE.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>> io/tst-closefrom.c | 15 +++-----------
>> posix/tst-spawn5.c | 13 +-----------
>> sysdeps/unix/sysv/linux/tst-close_range.c | 25 +++++++----------------
>> 3 files changed, 11 insertions(+), 42 deletions(-)
>>
>> diff --git a/io/tst-closefrom.c b/io/tst-closefrom.c
>> index d4c187073c..0800e19f3f 100644
>> --- a/io/tst-closefrom.c
>> +++ b/io/tst-closefrom.c
>> @@ -24,29 +24,20 @@
>> #include <support/check.h>
>> #include <support/descriptors.h>
>> #include <support/xunistd.h>
>> +#include <support/support.h>
>>
>> #include <array_length.h>
>>
>> #define NFDS 100
>>
>> -static int
>> -open_multiple_temp_files (void)
>> -{
>> - /* Check if the temporary file descriptor has no no gaps. */
>> - int lowfd = xopen ("/dev/null", O_RDONLY, 0600);
>> - for (int i = 1; i <= NFDS; i++)
>> - TEST_COMPARE (xopen ("/dev/null", O_RDONLY, 0600), lowfd + i);
>> - return lowfd;
>> -}
>> -
>> static int
>> closefrom_test (void)
>> {
>> struct support_descriptors *descrs = support_descriptors_list ();
>>
>> - int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>>
>> - const int maximum_fd = lowfd + NFDS;
>> + const int maximum_fd = lowfd + NFDS - 1;
>> const int half_fd = lowfd + NFDS / 2;
>> const int gap = maximum_fd / 4;
>>
>
> There are two for loops in this function that iterate from 0:
>
> for (int i = 0; i < half_fd; i++)
> TEST_VERIFY (fcntl (i, F_GETFL) > -1);
>
> for (int i = 0; i < gap; i++)
> TEST_VERIFY (fcntl (i, F_GETFL) > -1);
>
> These should both iterate from i = lowfd I think? (And maybe should have
> done before this change?) Certainly they fail in the cases this patch is
> trying to fix.
>
> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
>> index ac66738004..a95199af6b 100644
>> --- a/posix/tst-spawn5.c
>> +++ b/posix/tst-spawn5.c
>> @@ -47,17 +47,6 @@ static int initial_argv_count;
>>
>> #define NFDS 100
>>
>> -static int
>> -open_multiple_temp_files (void)
>> -{
>> - /* Check if the temporary file descriptor has no no gaps. */
>> - int lowfd = xopen ("/dev/null", O_RDONLY, 0600);
>> - for (int i = 1; i <= NFDS; i++)
>> - TEST_COMPARE (xopen ("/dev/null", O_RDONLY, 0600),
>> - lowfd + i);
>> - return lowfd;
>> -}
>> -
>> static int
>> parse_fd (const char *str)
>> {
>> @@ -185,7 +174,7 @@ spawn_closefrom_test (posix_spawn_file_actions_t *fa,
>> int lowfd, int highfd,
>> static void
>> do_test_closefrom (void)
>> {
>> - int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>> const int half_fd = lowfd + NFDS / 2;
>>
>> /* Close half of the descriptors and check result. */
>> diff --git a/sysdeps/unix/sysv/linux/tst-close_range.c
>> b/sysdeps/unix/sysv/linux/tst-close_range.c
>> index dccb6189c5..3548b10363 100644
>> --- a/sysdeps/unix/sysv/linux/tst-close_range.c
>> +++ b/sysdeps/unix/sysv/linux/tst-close_range.c
>>
>
> This for look in close_range_unshare_test in this file needs to change too:
>
> for (int i = 0; i < NFDS; i++)
> TEST_VERIFY (fcntl (i, F_GETFL) > -1);
>
> (presumably to iterate from low_fd to lowfd + NFDS).
>
>
>> @@ -36,23 +36,12 @@
>>
>> #define NFDS 100
>>
>> -static int
>> -open_multiple_temp_files (void)
>> -{
>> - /* Check if the temporary file descriptor has no no gaps. */
>> - int lowfd = xopen ("/dev/null", O_RDONLY, 0600);
>> - for (int i = 1; i <= NFDS; i++)
>> - TEST_COMPARE (xopen ("/dev/null", O_RDONLY, 0600),
>> - lowfd + i);
>> - return lowfd;
>> -}
>> -
>> static void
>> close_range_test_max_upper_limit (void)
>> {
>> struct support_descriptors *descrs = support_descriptors_list ();
>>
>> - int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>>
>> {
>> int r = close_range (lowfd, ~0U, 0);
>> @@ -68,7 +57,7 @@ close_range_test_max_upper_limit (void)
>> static void
>> close_range_test_common (int lowfd, unsigned int flags)
>> {
>> - const int maximum_fd = lowfd + NFDS;
>> + const int maximum_fd = lowfd + NFDS - 1;
>> const int half_fd = lowfd + NFDS / 2;
>> const int gap_1 = maximum_fd - 8;
>>
>> @@ -121,7 +110,7 @@ close_range_test (void)
>> struct support_descriptors *descrs = support_descriptors_list ();
>>
>> /* Check if the temporary file descriptor has no no gaps. */
>> - int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>>
>> close_range_test_common (lowfd, 0);
>>
>> @@ -146,7 +135,7 @@ close_range_test_subprocess (void)
>> struct support_descriptors *descrs = support_descriptors_list ();
>>
>> /* Check if the temporary file descriptor has no no gaps. */
>> - int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>>
>> struct support_stack stack = support_stack_alloc (4096);
>>
>> @@ -184,7 +173,7 @@ close_range_unshare_test (void)
>> struct support_descriptors *descrs1 = support_descriptors_list ();
>>
>> /* Check if the temporary file descriptor has no no gaps. */
>> - int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>>
>> struct support_descriptors *descrs2 = support_descriptors_list ();
>>
>> @@ -226,9 +215,9 @@ static void
>> close_range_cloexec_test (void)
>> {
>> /* Check if the temporary file descriptor has no no gaps. */
>> - const int lowfd = open_multiple_temp_files ();
>> + int lowfd = support_open_dev_null_range (NFDS, O_RDONLY, 0600);
>>
>> - const int maximum_fd = lowfd + NFDS;
>> + const int maximum_fd = lowfd + NFDS - 1;
>> const int half_fd = lowfd + NFDS / 2;
>> const int gap_1 = maximum_fd - 8;
>>
>> --
>> 2.30.2
>>
>
> Cheers,
> mwh
>
I'm attaching a patch which when applied on top of this one makes these
tests pass my hackish way of reproducing the issue:
mwhudson@anduril:~/src/pkg/build-glibc$ bash -c 'exec 40</dev/null;
LD_LIBRARY_PATH=. ./elf/ld-linux-x86-64.so.2 ./misc/tst-close_range'
mwhudson@anduril:~/src/pkg/build-glibc$ bash -c 'exec 40</dev/null;
LD_LIBRARY_PATH=. ./elf/ld-linux-x86-64.so.2 ./io/tst-closefrom'
Cheers,
mwh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mwhudson.patch
Type: text/x-patch
Size: 1203 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20210825/17cf78d8/attachment-0001.bin>