[PATCH v4] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]

[PATCH v4] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]

[Xiaoming Ni]

In ftw_startup(), call alloca to apply for a large amount of stack space.
When descriptors is very large, stack overflow is triggered. BZ #26353

Replace the memory application of data.dirstreams from alloca() to malloc()
by referring to the memory application of data.dirbuf in the current function.
Combine the memory allocation of data.dirbuf and data.dirstreams according
to the advice of Adhemerval Zanella.

v4:
  Based on Adhemerval Zanella's review comments,
  use support_capture_subprocess_check() to rewrite testcase
v3: https://public-inbox.org/libc-alpha/20201124131652.111854-1-nixiaoming@huawei.com/
  Combine the memory allocation of data.dirbuf and data.dirstreams
    according to the advice of Adhemerval Zanella.
  remove the upper limit check of descriptors
v2: https://public-inbox.org/libc-alpha/20200815070851.46403-1-nixiaoming@huawei.com/
  not set errno after malloc fails.
  add check ftw return value on testcase
  add more testcase
v1: https://public-inbox.org/libc-alpha/20200808084640.49174-1-nixiaoming@huawei.com/
'
---
 io/Makefile      |  3 +-
 io/ftw.c         | 16 ++++++-----
 io/tst-bz26353.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 8 deletions(-)
 create mode 100644 io/tst-bz26353.c

diff --git a/io/Makefile b/io/Makefile
index 6dd2c33fcf..7845d07f45 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -68,7 +68,8 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
 		   tst-posix_fallocate tst-posix_fallocate64 \
 		   tst-fts tst-fts-lfs tst-open-tmpfile \
 		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
-		   tst-ftw-lnk tst-file_change_detection tst-lchmod
+		   tst-ftw-lnk tst-file_change_detection tst-lchmod \
+		   tst-bz26353
 
 # Likewise for statx, but we do not need static linking here.
 tests-internal += tst-statx
diff --git a/io/ftw.c b/io/ftw.c
index 7104816e85..92e08c5431 100644
--- a/io/ftw.c
+++ b/io/ftw.c
@@ -646,15 +646,17 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
 
   data.maxdir = descriptors < 1 ? 1 : descriptors;
   data.actdir = 0;
-  data.dirstreams = (struct dir_data **) alloca (data.maxdir
-						 * sizeof (struct dir_data *));
-  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));
-
   /* PATH_MAX is always defined when we get here.  */
   data.dirbufsize = MAX (2 * strlen (dir), PATH_MAX);
-  data.dirbuf = (char *) malloc (data.dirbufsize);
-  if (data.dirbuf == NULL)
+  data.dirstreams = malloc (data.maxdir * sizeof (struct dir_data *)
+                            + data.dirbufsize);
+  if (data.dirstreams == NULL)
     return -1;
+
+  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));
+
+  data.dirbuf = (char *) data.dirstreams
+                + data.maxdir * sizeof (struct dir_data *);
   cp = __stpcpy (data.dirbuf, dir);
   /* Strip trailing slashes.  */
   while (cp > data.dirbuf + 1 && cp[-1] == '/')
@@ -803,7 +805,7 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
  out_fail:
   save_err = errno;
   __tdestroy (data.known_objects, free);
-  free (data.dirbuf);
+  free (data.dirstreams);
   __set_errno (save_err);
 
   return result;
diff --git a/io/tst-bz26353.c b/io/tst-bz26353.c
new file mode 100644
index 0000000000..27f50a8bdc
--- /dev/null
+++ b/io/tst-bz26353.c
@@ -0,0 +1,75 @@
+/* test ftw bz26353: Check whether stack overflow occurs when the value
+   of the nopenfd parameter is too large.
+
+   Copyright (C) 2020 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 <stdlib.h>
+#include <stdio.h>
+#include <ftw.h>
+#include <errno.h>
+#include <sys/resource.h>
+
+#include <support/temp_file.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+
+static int
+my_func (const char *file, const struct stat *sb, int flag)
+{
+  return 0;
+}
+
+static void
+do_ftw (void *unused)
+{
+  char *tempdir = support_create_temp_directory ("tst-bz26353");
+  struct rlimit r;
+  int large_nopenfd = 80 * 1024 * 1024;
+  int res = getrlimit (RLIMIT_STACK, &r);
+  if (res == 0)
+    {
+      if (r.rlim_cur == RLIM_INFINITY)
+        {
+          r.rlim_cur = 8 * 1024 * 1024;
+          setrlimit (RLIMIT_STACK, &r);
+        }
+      large_nopenfd = (int) r.rlim_cur;
+    }
+  int ret = ftw (tempdir, my_func, large_nopenfd);
+  if (ret != 0)
+    {
+      printf ("test big num %d, ret=%d errno=%d, without stack overflow\n",
+              large_nopenfd, ret, errno);
+    }
+  free (tempdir);
+  exit (ret);
+}
+
+/*Check whether stack overflow occurs*/
+static int
+do_test (void)
+{
+  struct support_capture_subprocess result;
+  result = support_capture_subprocess (do_ftw, NULL);
+  support_capture_subprocess_check (&result, "bz26353", 0, sc_allow_none);
+  support_capture_subprocess_free (&result);
+  return 0;
+}
+
+#include <support/test-driver.c>
+
-- 
2.27.0

Re: [PATCH v4] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]

[Adhemerval Zanella]

On 25/11/2020 07:06, Xiaoming Ni wrote:
> In ftw_startup(), call alloca to apply for a large amount of stack space.
> When descriptors is very large, stack overflow is triggered. BZ #26353
> 
> Replace the memory application of data.dirstreams from alloca() to malloc()
> by referring to the memory application of data.dirbuf in the current function.
> Combine the memory allocation of data.dirbuf and data.dirstreams according
> to the advice of Adhemerval Zanella.
> 
> v4:
>   Based on Adhemerval Zanella's review comments,
>   use support_capture_subprocess_check() to rewrite testcase
> v3: https://public-inbox.org/libc-alpha/20201124131652.111854-1-nixiaoming@huawei.com/
>   Combine the memory allocation of data.dirbuf and data.dirstreams
>     according to the advice of Adhemerval Zanella.
>   remove the upper limit check of descriptors
> v2: https://public-inbox.org/libc-alpha/20200815070851.46403-1-nixiaoming@huawei.com/
>   not set errno after malloc fails.
>   add check ftw return value on testcase
>   add more testcase
> v1: https://public-inbox.org/libc-alpha/20200808084640.49174-1-nixiaoming@huawei.com/
> '

Looks good, with a small adjustment below.  If you are ok with my
suggestion I can push on master on your behalf.

Thanks.

> ---
>  io/Makefile      |  3 +-
>  io/ftw.c         | 16 ++++++-----
>  io/tst-bz26353.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 8 deletions(-)
>  create mode 100644 io/tst-bz26353.c
> 
> diff --git a/io/Makefile b/io/Makefile
> index 6dd2c33fcf..7845d07f45 100644
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -68,7 +68,8 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
>  		   tst-posix_fallocate tst-posix_fallocate64 \
>  		   tst-fts tst-fts-lfs tst-open-tmpfile \
>  		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
> -		   tst-ftw-lnk tst-file_change_detection tst-lchmod
> +		   tst-ftw-lnk tst-file_change_detection tst-lchmod \
> +		   tst-bz26353
>  
>  # Likewise for statx, but we do not need static linking here.
>  tests-internal += tst-statx

Ok.

> diff --git a/io/ftw.c b/io/ftw.c
> index 7104816e85..92e08c5431 100644
> --- a/io/ftw.c
> +++ b/io/ftw.c
> @@ -646,15 +646,17 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
>  
>    data.maxdir = descriptors < 1 ? 1 : descriptors;
>    data.actdir = 0;
> -  data.dirstreams = (struct dir_data **) alloca (data.maxdir
> -						 * sizeof (struct dir_data *));
> -  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));
> -
>    /* PATH_MAX is always defined when we get here.  */
>    data.dirbufsize = MAX (2 * strlen (dir), PATH_MAX);
> -  data.dirbuf = (char *) malloc (data.dirbufsize);
> -  if (data.dirbuf == NULL)
> +  data.dirstreams = malloc (data.maxdir * sizeof (struct dir_data *)
> +                            + data.dirbufsize);
> +  if (data.dirstreams == NULL)
>      return -1;
> +
> +  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));
> +
> +  data.dirbuf = (char *) data.dirstreams
> +                + data.maxdir * sizeof (struct dir_data *);
>    cp = __stpcpy (data.dirbuf, dir);
>    /* Strip trailing slashes.  */
>    while (cp > data.dirbuf + 1 && cp[-1] == '/')

Ok.

> @@ -803,7 +805,7 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
>   out_fail:
>    save_err = errno;
>    __tdestroy (data.known_objects, free);
> -  free (data.dirbuf);
> +  free (data.dirstreams);
>    __set_errno (save_err);
>  
>    return result;

Ok.

> diff --git a/io/tst-bz26353.c b/io/tst-bz26353.c
> new file mode 100644
> index 0000000000..27f50a8bdc
> --- /dev/null
> +++ b/io/tst-bz26353.c
> @@ -0,0 +1,75 @@
> +/* test ftw bz26353: Check whether stack overflow occurs when the value
> +   of the nopenfd parameter is too large.
> +
> +   Copyright (C) 2020 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 <stdlib.h>
> +#include <stdio.h>
> +#include <ftw.h>
> +#include <errno.h>
> +#include <sys/resource.h>
> +
> +#include <support/temp_file.h>
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +
> +static int
> +my_func (const char *file, const struct stat *sb, int flag)
> +{
> +  return 0;
> +}
> +
> +static void
> +do_ftw (void *unused)
> +{
> +  char *tempdir = support_create_temp_directory ("tst-bz26353");
> +  struct rlimit r;
> +  int large_nopenfd = 80 * 1024 * 1024;

This is will always overwritten by the next assignment, getrlimit
will always succeed, unless it is being filtered by seccomp or
similar facility.  In any case, other glibc will also fail, so we
can assume it always available.

> +  int res = getrlimit (RLIMIT_STACK, &r);
> +  if (res == 0)

Just check the result with TEST_COMPARE.

> +    {
> +      if (r.rlim_cur == RLIM_INFINITY)
> +        {
> +          r.rlim_cur = 8 * 1024 * 1024;
> +          setrlimit (RLIMIT_STACK, &r);
> +        }
> +      large_nopenfd = (int) r.rlim_cur;

Ok, it limiting the stack to 8 MB in any case, otherwise it is
setting a fd value that will likely trigger a stack overflow.

> +    }
> +  int ret = ftw (tempdir, my_func, large_nopenfd);
> +  if (ret != 0)
> +    {
> +      printf ("test big num %d, ret=%d errno=%d, without stack overflow\n",
> +              large_nopenfd, ret, errno);
> +    }

Just check with TEST_COMPARE where.

I a short I think it can be simplified with:

  struct rlimit r;
  TEST_COMPARE (getrlimit (RLIMIT_STACK, &r), 0);
  if (r.rlim_cur == RLIM_INFINITY)
    { 
      r.rlim_cur = 8 * 1024 * 1024;
      TEST_COMPARE (setrlimit (RLIMIT_STACK, &r), 0);
    }
  int large_nopenfd = (int) r.rlim_cur;


> +  free (tempdir);
> +  exit (ret);
> +}
> +
> +/*Check whether stack overflow occurs*/
> +static int
> +do_test (void)
> +{
> +  struct support_capture_subprocess result;
> +  result = support_capture_subprocess (do_ftw, NULL);
> +  support_capture_subprocess_check (&result, "bz26353", 0, sc_allow_none);
> +  support_capture_subprocess_free (&result);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> +

Spurious extra line.

Re: [PATCH v4] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]

[Xiaoming Ni]

On 2020/11/25 23:21, Adhemerval Zanella wrote:
> 
> 
> On 25/11/2020 07:06, Xiaoming Ni wrote:
>> In ftw_startup(), call alloca to apply for a large amount of stack space.
>> When descriptors is very large, stack overflow is triggered. BZ #26353
>>
>> Replace the memory application of data.dirstreams from alloca() to malloc()
>> by referring to the memory application of data.dirbuf in the current function.
>> Combine the memory allocation of data.dirbuf and data.dirstreams according
>> to the advice of Adhemerval Zanella.
>>
>> v4:
>>    Based on Adhemerval Zanella's review comments,
>>    use support_capture_subprocess_check() to rewrite testcase
>> v3: https://public-inbox.org/libc-alpha/20201124131652.111854-1-nixiaoming@huawei.com/
>>    Combine the memory allocation of data.dirbuf and data.dirstreams
>>      according to the advice of Adhemerval Zanella.
>>    remove the upper limit check of descriptors
>> v2: https://public-inbox.org/libc-alpha/20200815070851.46403-1-nixiaoming@huawei.com/
>>    not set errno after malloc fails.
>>    add check ftw return value on testcase
>>    add more testcase
>> v1: https://public-inbox.org/libc-alpha/20200808084640.49174-1-nixiaoming@huawei.com/
>> '
> 
> Looks good, with a small adjustment below.  If you are ok with my
> suggestion I can push on master on your behalf.
> 
> Thanks.
> 
>> ---
>>   io/Makefile      |  3 +-
>>   io/ftw.c         | 16 ++++++-----
>>   io/tst-bz26353.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 86 insertions(+), 8 deletions(-)
>>   create mode 100644 io/tst-bz26353.c
>>
>> diff --git a/io/Makefile b/io/Makefile
>> index 6dd2c33fcf..7845d07f45 100644
>> --- a/io/Makefile
>> +++ b/io/Makefile
>> @@ -68,7 +68,8 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
>>   		   tst-posix_fallocate tst-posix_fallocate64 \
>>   		   tst-fts tst-fts-lfs tst-open-tmpfile \
>>   		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
>> -		   tst-ftw-lnk tst-file_change_detection tst-lchmod
>> +		   tst-ftw-lnk tst-file_change_detection tst-lchmod \
>> +		   tst-bz26353
>>   
>>   # Likewise for statx, but we do not need static linking here.
>>   tests-internal += tst-statx
> 
> Ok.
> 
>> diff --git a/io/ftw.c b/io/ftw.c
>> index 7104816e85..92e08c5431 100644
>> --- a/io/ftw.c
>> +++ b/io/ftw.c
>> @@ -646,15 +646,17 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
>>   
>>     data.maxdir = descriptors < 1 ? 1 : descriptors;
>>     data.actdir = 0;
>> -  data.dirstreams = (struct dir_data **) alloca (data.maxdir
>> -						 * sizeof (struct dir_data *));
>> -  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));
>> -
>>     /* PATH_MAX is always defined when we get here.  */
>>     data.dirbufsize = MAX (2 * strlen (dir), PATH_MAX);
>> -  data.dirbuf = (char *) malloc (data.dirbufsize);
>> -  if (data.dirbuf == NULL)
>> +  data.dirstreams = malloc (data.maxdir * sizeof (struct dir_data *)
>> +                            + data.dirbufsize);
>> +  if (data.dirstreams == NULL)
>>       return -1;
>> +
>> +  memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *));
>> +
>> +  data.dirbuf = (char *) data.dirstreams
>> +                + data.maxdir * sizeof (struct dir_data *);
>>     cp = __stpcpy (data.dirbuf, dir);
>>     /* Strip trailing slashes.  */
>>     while (cp > data.dirbuf + 1 && cp[-1] == '/')
> 
> Ok.
> 
>> @@ -803,7 +805,7 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
>>    out_fail:
>>     save_err = errno;
>>     __tdestroy (data.known_objects, free);
>> -  free (data.dirbuf);
>> +  free (data.dirstreams);
>>     __set_errno (save_err);
>>   
>>     return result;
> 
> Ok.
> 
>> diff --git a/io/tst-bz26353.c b/io/tst-bz26353.c
>> new file mode 100644
>> index 0000000000..27f50a8bdc
>> --- /dev/null
>> +++ b/io/tst-bz26353.c
>> @@ -0,0 +1,75 @@
>> +/* test ftw bz26353: Check whether stack overflow occurs when the value
>> +   of the nopenfd parameter is too large.
>> +
>> +   Copyright (C) 2020 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 <stdlib.h>
>> +#include <stdio.h>
>> +#include <ftw.h>
>> +#include <errno.h>
>> +#include <sys/resource.h>
>> +
>> +#include <support/temp_file.h>
>> +#include <support/capture_subprocess.h>
>> +#include <support/check.h>
>> +
>> +static int
>> +my_func (const char *file, const struct stat *sb, int flag)
>> +{
>> +  return 0;
>> +}
>> +
>> +static void
>> +do_ftw (void *unused)
>> +{
>> +  char *tempdir = support_create_temp_directory ("tst-bz26353");
>> +  struct rlimit r;
>> +  int large_nopenfd = 80 * 1024 * 1024;
> 
> This is will always overwritten by the next assignment, getrlimit
> will always succeed, unless it is being filtered by seccomp or
> similar facility.  In any case, other glibc will also fail, so we
> can assume it always available.
> 
>> +  int res = getrlimit (RLIMIT_STACK, &r);
>> +  if (res == 0)
> 
> Just check the result with TEST_COMPARE.
> 
>> +    {
>> +      if (r.rlim_cur == RLIM_INFINITY)
>> +        {
>> +          r.rlim_cur = 8 * 1024 * 1024;
>> +          setrlimit (RLIMIT_STACK, &r);
>> +        }
>> +      large_nopenfd = (int) r.rlim_cur;
> 
> Ok, it limiting the stack to 8 MB in any case, otherwise it is
> setting a fd value that will likely trigger a stack overflow.
> 
>> +    }
>> +  int ret = ftw (tempdir, my_func, large_nopenfd);
>> +  if (ret != 0)
>> +    {
>> +      printf ("test big num %d, ret=%d errno=%d, without stack overflow\n",
>> +              large_nopenfd, ret, errno);
>> +    }
> 
> Just check with TEST_COMPARE where.
> 
> I a short I think it can be simplified with:
> 
>    struct rlimit r;
>    TEST_COMPARE (getrlimit (RLIMIT_STACK, &r), 0);
>    if (r.rlim_cur == RLIM_INFINITY)
>      {
>        r.rlim_cur = 8 * 1024 * 1024;
>        TEST_COMPARE (setrlimit (RLIMIT_STACK, &r), 0);
>      }
>    int large_nopenfd = (int) r.rlim_cur;
> 
Thank you for your guidance.
I will modify and send the v5 patch later based on your review comments.

Thanks
Xiaoming Ni

> 
>> +  free (tempdir);
>> +  exit (ret);
>> +}
>> +
>> +/*Check whether stack overflow occurs*/
>> +static int
>> +do_test (void)
>> +{
>> +  struct support_capture_subprocess result;
>> +  result = support_capture_subprocess (do_ftw, NULL);
>> +  support_capture_subprocess_check (&result, "bz26353", 0, sc_allow_none);
>> +  support_capture_subprocess_free (&result);
>> +  return 0;
>> +}
>> +
>> +#include <support/test-driver.c>
>> +
> 
> Spurious extra line.
> .
>