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

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