[PATCH 0/2] Fix close_range/closefrom tests

[PATCH 0/2] Fix close_range/closefrom tests

[Adhemerval Zanella]

This patchset fixes io/tst-closefrom, misc/tst-close_range,
posix/tst-spawn5 tests on some environment where the tests hit the
file descriptors limit or if there is already file descriptor
opened within the range created by the tests.

Adhemerval Zanella (2):
  support: Add support_open_dev_null_range
  Use support_open_dev_null_range io/tst-closefrom,
    misc/tst-close_range, and posix/tst-spawn5 (BZ #28260)

 io/tst-closefrom.c                        |  15 +--
 posix/tst-spawn5.c                        |  13 +-
 support/Makefile                          |   2 +
 support/support-open-dev-null-range.c     | 134 +++++++++++++++++++++
 support/support.h                         |   8 ++
 support/tst-support-open-dev-null-range.c | 137 ++++++++++++++++++++++
 sysdeps/unix/sysv/linux/tst-close_range.c |  25 ++--
 7 files changed, 292 insertions(+), 42 deletions(-)
 create mode 100644 support/support-open-dev-null-range.c
 create mode 100644 support/tst-support-open-dev-null-range.c

-- 
2.30.2

[PATCH 1/2] support: Add support_open_dev_null_range

[Adhemerval Zanella]

It returns a range of file descriptor referring to the '/dev/null'
pathname.  The function takes care of restarting the open range
if a file descriptor is found within the specified range and
also increases RLIMIT_NOFILE if required.

Checked on x86_64-linux-gnu.
---
 support/Makefile                          |   2 +
 support/support-open-dev-null-range.c     | 134 +++++++++++++++++++++
 support/support.h                         |   8 ++
 support/tst-support-open-dev-null-range.c | 137 ++++++++++++++++++++++
 4 files changed, 281 insertions(+)
 create mode 100644 support/support-open-dev-null-range.c
 create mode 100644 support/tst-support-open-dev-null-range.c

diff --git a/support/Makefile b/support/Makefile
index a462781718..6332e7b607 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -66,6 +66,7 @@ libsupport-routines = \
   support_path_support_time64 \
   support_process_state \
   support_ptrace \
+  support-open-dev-null-range \
   support_openpty \
   support_paths \
   support_quote_blob \
@@ -264,6 +265,7 @@ tests = \
   tst-support_capture_subprocess \
   tst-support_descriptors \
   tst-support_format_dns_packet \
+  tst-support-open-dev-null-range \
   tst-support-process_state \
   tst-support_quote_blob \
   tst-support_quote_string \
diff --git a/support/support-open-dev-null-range.c b/support/support-open-dev-null-range.c
new file mode 100644
index 0000000000..fe82d3b9f2
--- /dev/null
+++ b/support/support-open-dev-null-range.c
@@ -0,0 +1,134 @@
+/* Return a range of open file descriptors.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <support/support.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+#include <stdlib.h>
+#include <sys/resource.h>
+
+static void
+increase_nofile (void)
+{
+  struct rlimit rl;
+  if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
+    FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m");
+
+  rl.rlim_cur += 128;
+
+  if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
+    FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m");
+}
+
+static int
+open_dev_null (int flags, mode_t mode)
+{
+ int fd = open64 ("/dev/null", flags, mode);
+ if (fd > 0)
+   return fd;
+
+ if (fd < 0 && errno != EMFILE)
+   FAIL_EXIT1 ("open64 (\"/dev/null\", 0x%x, 0%o): %m", flags, mode);
+
+ increase_nofile ();
+
+ return xopen ("/dev/null", flags, mode);
+}
+
+struct range
+{
+  int lowfd;
+  size_t len;
+};
+
+struct range_list
+{
+  size_t total;
+  size_t used;
+  struct range *ranges;
+};
+
+static void
+range_init (struct range_list *r)
+{
+  r->total = 8;
+  r->used = 0;
+  r->ranges = xmalloc (8 * sizeof (struct range));
+}
+
+static void
+range_add (struct range_list *r, int lowfd, size_t len)
+{
+  if (r->used == r->total)
+    {
+      r->total *= 2;
+      r->ranges = xrealloc (r->ranges, r->total);
+    }
+  r->ranges[r->used].lowfd = lowfd;
+  r->ranges[r->used].len = len;
+  r->used++;
+}
+
+static void
+range_close (struct range_list *r)
+{
+  for (size_t i = 0; i < r->used; i++)
+    {
+      int minfd = r->ranges[i].lowfd;
+      int maxfd = r->ranges[i].lowfd + r->ranges[i].len;
+      for (int fd = minfd; fd < maxfd; fd++)
+	xclose (fd);
+    }
+  free (r->ranges);
+}
+
+int
+support_open_dev_null_range (int num, int flags, mode_t mode)
+{
+  /* We keep track of the ranges that hit an already opened descriptor, so
+     we close them after we get a working range.  */
+  struct range_list rl;
+  range_init (&rl);
+
+  int lowfd = open_dev_null (flags, mode);
+  int prevfd = lowfd;
+  while (true)
+    {
+      int i = 1;
+      for (; i < num; i++)
+	{
+	  int fd = open_dev_null (flags, mode);
+	  if (fd != lowfd + i)
+	    {
+	      range_add (&rl, lowfd, prevfd - lowfd + 1);
+
+	      prevfd = lowfd = fd;
+	      break;
+	    }
+	  prevfd = fd;
+	}
+      if (i == num)
+	break;
+    }
+
+  range_close (&rl);
+
+  return lowfd;
+}
diff --git a/support/support.h b/support/support.h
index 834dba9097..bffbb10f0d 100644
--- a/support/support.h
+++ b/support/support.h
@@ -193,6 +193,14 @@ struct support_stack support_stack_alloc (size_t size);
 /* Deallocate the STACK.  */
 void support_stack_free (struct support_stack *stack);
 
+
+/* Create a range of NUM opened '/dev/null' file descriptors using FLAGS and
+   MODE.  The function takes care of restarting the open range if a file
+   descriptor is found within the specified range and also increases
+   RLIMIT_NOFILE if required. 
+   The returned value is the lowest file descriptor number.  */
+int support_open_dev_null_range (int num, int flags, mode_t mode);
+
 __END_DECLS
 
 #endif /* SUPPORT_H */
diff --git a/support/tst-support-open-dev-null-range.c b/support/tst-support-open-dev-null-range.c
new file mode 100644
index 0000000000..8be430ea2a
--- /dev/null
+++ b/support/tst-support-open-dev-null-range.c
@@ -0,0 +1,137 @@
+/* Tests for support_open_dev_null_range.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xunistd.h>
+#include <sys/resource.h>
+#include <stdlib.h>
+
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
+#include <stdio.h>
+
+static void
+check_path (int fd)
+{
+  char *proc_fd_path = xasprintf ("/proc/self/fd/%d", fd);
+  char file_path[PATH_MAX];
+  ssize_t file_path_length
+    = readlink (proc_fd_path, file_path, sizeof (file_path));
+  if (file_path_length < 0)
+    FAIL_EXIT1 ("readlink (%s, %p, %zu)", proc_fd_path, file_path,
+		sizeof (file_path));
+  file_path[file_path_length] = '\0';
+  TEST_COMPARE_STRING (file_path, "/dev/null");
+}
+
+static int
+number_of_opened_files (void)
+{
+  DIR *fds = opendir ("/proc/self/fd");
+  if (fds == NULL)
+    FAIL_EXIT1 ("opendir (\"/proc/self/fd\"): %m");
+
+  int r = 0;
+  while (true)
+    {
+      errno = 0;
+      struct dirent64 *e = readdir64 (fds);
+      if (e == NULL)
+        {
+          if (errno != 0)
+            FAIL_EXIT1 ("readdir: %m");
+          break;
+        }
+
+      if (e->d_name[0] == '.')
+        continue;
+
+      char *endptr;
+      long int fd = strtol (e->d_name, &endptr, 10);
+      if (*endptr != '\0' || fd < 0 || fd > INT_MAX)
+        FAIL_EXIT1 ("readdir: invalid file descriptor name: /proc/self/fd/%s",
+                    e->d_name);
+
+      /* Skip the descriptor which is used to enumerate the
+         descriptors.  */
+      if (fd == dirfd (fds))
+        continue;
+
+      r = r + 1;
+    }
+
+  closedir (fds);
+
+  return r;
+}
+
+static int
+do_test (void)
+{
+  const int nfds1 = 8;
+  int lowfd = support_open_dev_null_range (nfds1, O_RDONLY, 0600);
+  for (int i = 0; i < nfds1; i++)
+    {
+      TEST_VERIFY (fcntl (lowfd + i, F_GETFL) > -1);
+      check_path (lowfd + i);
+    }
+
+  /* create some gaps.  */
+  xclose (lowfd + 1);
+  xclose (lowfd + 5);
+  xclose (lowfd + 6);
+
+  const int nfds2 = 16;
+  int lowfd2 = support_open_dev_null_range (nfds2, O_RDONLY, 0600);
+  for (int i = 0; i < nfds2; i++)
+    {
+      TEST_VERIFY (fcntl (lowfd2 + i, F_GETFL) > -1);
+      check_path (lowfd2 + i);
+    }
+
+  /* Decrease the maximum number of files.  */
+  {
+    struct rlimit rl;
+    if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
+      FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m");
+    
+    rl.rlim_cur = number_of_opened_files ();
+
+    if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
+      FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m");
+  }
+
+  const int nfds3 = 16;
+  int lowfd3 = support_open_dev_null_range (nfds3, O_RDONLY, 0600);
+  for (int i = 0; i < nfds3; i++)
+    {
+      TEST_VERIFY (fcntl (lowfd3 + i, F_GETFL) > -1);
+      check_path (lowfd3 + i);
+    }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

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

[Adhemerval Zanella]

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

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]

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

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]

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

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: 1203 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);

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]

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

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]

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.

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]

Hi,

On Wed, 25 Aug 2021 at 23:46, Adhemerval Zanella <
adhemerval.zanella@linaro.org> wrote:

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

Thanks for committing this to master. Should it be backported to the 2.34
branch too?

Cheers,
mwh

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

[Florian Weimer]

* Michael Hudson-Doyle via Libc-alpha:

> On Wed, 25 Aug 2021 at 23:46, Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
>
>> Thanks for catching it, I add merged on the patch and I will commit this
>> shortly.
>>
>
> Thanks for committing this to master. Should it be backported to the
> 2.34 branch too?

Backporting makes sense to me.  Are you going to take care of it?

Thanks,
Florian

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]

On Fri, 17 Sept 2021 at 00:09, Florian Weimer <fweimer@redhat.com> wrote:

> * Michael Hudson-Doyle via Libc-alpha:
>
> > On Wed, 25 Aug 2021 at 23:46, Adhemerval Zanella <
> > adhemerval.zanella@linaro.org> wrote:
> >
> >> Thanks for catching it, I add merged on the patch and I will commit this
> >> shortly.
> >>
> >
> > Thanks for committing this to master. Should it be backported to the
> > 2.34 branch too?
>
> Backporting makes sense to me.  Are you going to take care of it?
>

What does taking care of it mean in this context? I can't commit it, but I
can confirm that the patches cherry-pick cleanly to release/2.34/master. Is
it expected to file a bug in bugzilla for a backport request?

Cheers,
mwh

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

[Florian Weimer]

* Michael Hudson-Doyle:

> What does taking care of it mean in this context? I can't commit it,
> but I can confirm that the patches cherry-pick cleanly to
> release/2.34/master. Is it expected to file a bug in bugzilla for a
> backport request?

I think you could apply for commit rights so that you can do the
backport yourself (basically, “git cherry-pick -x” it, test it, then
push to sourceware).

What do you think?

Thanks,
Florian

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]

On Fri, 17 Sept 2021 at 22:12, Florian Weimer <fweimer@redhat.com> wrote:

> * Michael Hudson-Doyle:
>
> > What does taking care of it mean in this context? I can't commit it,
> > but I can confirm that the patches cherry-pick cleanly to
> > release/2.34/master. Is it expected to file a bug in bugzilla for a
> > backport request?
>
> I think you could apply for commit rights so that you can do the
> backport yourself (basically, “git cherry-pick -x” it, test it, then
> push to sourceware).
>

OK, sure!


> What do you think?
>

I found this guide:
https://sourceware.org/glibc/wiki/MAINTAINERS#Becoming_a_maintainer_.28developer.29
which perhaps is a little out of date wrt copyright assignments? (Although
I'm 99.99% certain Canonical has a copyright assignment on file with the
FSF anyway). I've created a patchwork account (mwhudson) and a wiki account
(MichaelHudsonDoyle), I think the next step would be to fill out this form?
https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Cheers,
mwh

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

[Florian Weimer]

* Michael Hudson-Doyle:

> I found this guide:
> https://sourceware.org/glibc/wiki/MAINTAINERS#Becoming_a_maintainer_.28developer.29
> which perhaps is a little out of date wrt copyright assignments?
> (Although I'm 99.99% certain Canonical has a copyright assignment on
> file with the FSF anyway). I've created a patchwork account (mwhudson)
> and a wiki account (MichaelHudsonDoyle), I think the next step would
> be to fill out this form?
> https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Yes, that's the right form to fill out.  You can list me as the
approver.

In the meantime, it might make sense to check internally if your
contributions will be covered by Canonical assignment, so that you know
whether posted patches should have Signed-off-by: or not.

Thanks,
Florian

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]

On Mon, 20 Sept 2021 at 22:55, Florian Weimer <fweimer@redhat.com> wrote:

> * Michael Hudson-Doyle:
>
> > I found this guide:
> >
> https://sourceware.org/glibc/wiki/MAINTAINERS#Becoming_a_maintainer_.28developer.29
> > which perhaps is a little out of date wrt copyright assignments?
> > (Although I'm 99.99% certain Canonical has a copyright assignment on
> > file with the FSF anyway). I've created a patchwork account (mwhudson)
> > and a wiki account (MichaelHudsonDoyle), I think the next step would
> > be to fill out this form?
> > https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>
> Yes, that's the right form to fill out.  You can list me as the
> approver.
>

Done, thanks.


> In the meantime, it might make sense to check internally if your
> contributions will be covered by Canonical assignment, so that you know
> whether posted patches should have Signed-off-by: or not.
>

I've asked about this, hopefully will find out soon. I guess it's not
needed to cherry-pick something to 2.34/master!

Cheers,
mwh

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]

On Tue, 21 Sept 2021 at 11:20, Michael Hudson-Doyle <
michael.hudson@canonical.com> wrote:

> On Mon, 20 Sept 2021 at 22:55, Florian Weimer <fweimer@redhat.com> wrote:
>
>> * Michael Hudson-Doyle:
>>
>> > I found this guide:
>> >
>> https://sourceware.org/glibc/wiki/MAINTAINERS#Becoming_a_maintainer_.28developer.29
>> > which perhaps is a little out of date wrt copyright assignments?
>> > (Although I'm 99.99% certain Canonical has a copyright assignment on
>> > file with the FSF anyway). I've created a patchwork account (mwhudson)
>> > and a wiki account (MichaelHudsonDoyle), I think the next step would
>> > be to fill out this form?
>> > https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>>
>> Yes, that's the right form to fill out.  You can list me as the
>> approver.
>>
>
> Done, thanks.
>
>
>> In the meantime, it might make sense to check internally if your
>> contributions will be covered by Canonical assignment, so that you know
>> whether posted patches should have Signed-off-by: or not.
>>
>
> I've asked about this, hopefully will find out soon. I guess it's not
> needed to cherry-pick something to 2.34/master!
>

OK, I've pushed these commits to release/2.34/master and emailed
libc-stable. Thanks for the support!

I'm not super familiar with email-based git development so apologies in
advance for any mis-steps...

Cheers,
mwh