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

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 25/08/2021 05:40, Michael Hudson-Doyle wrote:
> I found some more problems, updated patch attached.
> 
> On Wed, 25 Aug 2021 at 15:41, Michael Hudson-Doyle <michael.hudson@canonical.com <mailto:michael.hudson@canonical.com>> wrote:
> 
> 
> 
>     On Wed, 25 Aug 2021 at 15:15, Michael Hudson-Doyle <michael.hudson@canonical.com <mailto: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 <mailto: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
> 

Thanks for catching it, I add merged on the patch and I will commit this shortly.