diff mbox series

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

Message ID 20210824192858.681680-3-adhemerval.zanella@linaro.org
State Committed
Headers
Series Fix close_range/closefrom tests |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Adhemerval Zanella Aug. 24, 2021, 7:28 p.m. UTC 
  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(-)

Comments

Michael Hudson-Doyle Aug. 25, 2021, 3:15 a.m. UTC | #1 
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
Michael Hudson-Doyle Aug. 25, 2021, 3:41 a.m. UTC | #2 
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
Michael Hudson-Doyle Aug. 25, 2021, 8:40 a.m. UTC | #3 
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
>
Adhemerval Zanella Aug. 25, 2021, 11:46 a.m. UTC | #4 
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.
Michael Hudson-Doyle Sept. 14, 2021, 12:46 a.m. UTC | #5 
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
Florian Weimer Sept. 16, 2021, 12:09 p.m. UTC | #6 
* 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
Michael Hudson-Doyle Sept. 16, 2021, 11:44 p.m. UTC | #7 
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
Florian Weimer Sept. 17, 2021, 10:12 a.m. UTC | #8 
* 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
Michael Hudson-Doyle Sept. 19, 2021, 9:57 p.m. UTC | #9 
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
Florian Weimer Sept. 20, 2021, 10:54 a.m. UTC | #10 
* 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
Michael Hudson-Doyle Sept. 20, 2021, 11:20 p.m. UTC | #11 
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
Michael Hudson-Doyle Sept. 21, 2021, 10:30 a.m. UTC | #12 
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
diff mbox series

Patch

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;