Re: [PATCH 2/2] Use support_open_dev_null_range io/tst-closefrom, misc/tst-close_range, and posix/tst-spawn5 (BZ #28260)

[Michael Hudson-Doyle via Libc-alpha <libc-alpha@sourceware.org>]

[-- Attachment #1: Type: text/plain, Size: 7041 bytes --]

I found some more problems, updated patch attached.

On Wed, 25 Aug 2021 at 15:41, Michael Hudson-Doyle <
michael.hudson@canonical.com> wrote:

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

[-- Attachment #2: mwhudson.patch --]
[-- Type: text/x-patch, Size: 1835 bytes --]

--- a/io/tst-closefrom.c
+++ b/io/tst-closefrom.c
@@ -39,7 +39,7 @@
 
   const int maximum_fd = lowfd + NFDS - 1;
   const int half_fd = lowfd + NFDS / 2;
-  const int gap = maximum_fd / 4;
+  const int gap = lowfd + NFDS / 4;
 
   /* Close half of the descriptors and check result.  */
   closefrom (half_fd);
@@ -49,7 +49,7 @@
       TEST_COMPARE (fcntl (i, F_GETFL), -1);
       TEST_COMPARE (errno, EBADF);
     }
-  for (int i = 0; i < half_fd; i++)
+  for (int i = lowfd; i < half_fd; i++)
     TEST_VERIFY (fcntl (i, F_GETFL) > -1);
 
   /* Create some gaps, close up to a threshold, and check result.  */
@@ -65,7 +65,7 @@
       TEST_COMPARE (fcntl (i, F_GETFL), -1);
       TEST_COMPARE (errno, EBADF);
     }
-  for (int i = 0; i < gap; i++)
+  for (int i = lowfd; i < gap; i++)
     TEST_VERIFY (fcntl (i, F_GETFL) > -1);
 
   /* Close the remmaining but the last one.  */
--- a/sysdeps/unix/sysv/linux/tst-close_range.c
+++ b/sysdeps/unix/sysv/linux/tst-close_range.c
@@ -189,7 +189,7 @@
 
   support_stack_free (&stack);
 
-  for (int i = 0; i < NFDS; i++)
+  for (int i = lowfd; i < lowfd + NFDS; i++)
     TEST_VERIFY (fcntl (i, F_GETFL) > -1);
 
   support_descriptors_check (descrs2);
@@ -240,13 +240,13 @@
   /* Create some gaps, close up to a threshold, and check result.  */
   static int gap_close[] = { 57, 78, 81, 82, 84, 90 };
   for (int i = 0; i < array_length (gap_close); i++)
-    xclose (gap_close[i]);
+    xclose (lowfd + gap_close[i]);
 
   TEST_COMPARE (close_range (half_fd + 1, gap_1, CLOSE_RANGE_CLOEXEC), 0);
   for (int i = half_fd + 1; i < gap_1; i++)
     {
       int flags = fcntl (i, F_GETFD);
-      if (is_in_array (gap_close, array_length (gap_close), i))
+      if (is_in_array (gap_close, array_length (gap_close), i - lowfd))
         TEST_COMPARE (flags, -1);
       else
         {