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

[PATCH v3] 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.

v3:
  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         | 18 ++++++++++--------
 io/tst-bz26353.c | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 9 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..c38e94dffc 100644
--- a/io/ftw.c
+++ b/io/ftw.c
@@ -72,6 +72,7 @@ char *alloca ();
 #else
 # include <sys/stat.h>
 #endif
+#include <scratch_buffer.h>
 
 #if ! _LIBC && !HAVE_DECL_STPCPY && !defined stpcpy
 char *stpcpy ();
@@ -643,18 +644,19 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
       __set_errno (ENOENT);
       return -1;
     }
-
   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..7567d9c038
--- /dev/null
+++ b/io/tst-bz26353.c
@@ -0,0 +1,34 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <ftw.h>
+#include <sys/stat.h>
+#include <errno.h>
+#include <limits.h>
+
+int my_func(const char *file, const struct stat *sb ,int flag)
+{
+  printf ("%s\n", file);
+  return 0;
+}
+
+/*Check whether stack overflow occurs*/
+int do_test(int large_nopenfd)
+{
+  int ret = ftw("./tst-bz26353", my_func, large_nopenfd);
+  printf ("test big num %d, ret=%d errno=%d, without stack overflow\n",
+          large_nopenfd, ret, errno);
+  return 0;
+}
+
+int main(int argc, char *argv[])
+{
+  int ret = 0;
+  mkdir ("./tst-bz26353", 0755);
+  /* The default stack size is 8 MB.
+   * Before the bug is fixed, stack overflow will be triggered. */
+  ret += do_test(8192 * 1024);
+  ret += do_test(INT_MAX);
+  rmdir ("./tst-bz26353");
+  return ret;
+}
-- 
2.27.0

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

[Adhemerval Zanella]

On 24/11/2020 10:16, 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.
> 
> v3:
>   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         | 18 ++++++++++--------
>  io/tst-bz26353.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 9 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..c38e94dffc 100644
> --- a/io/ftw.c
> +++ b/io/ftw.c
> @@ -72,6 +72,7 @@ char *alloca ();
>  #else
>  # include <sys/stat.h>
>  #endif
> +#include <scratch_buffer.h>

This header is not needed.

>  
>  #if ! _LIBC && !HAVE_DECL_STPCPY && !defined stpcpy
>  char *stpcpy ();
> @@ -643,18 +644,19 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors,
>        __set_errno (ENOENT);
>        return -1;
>      }
> -

Spurious line removal.

>    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..7567d9c038
> --- /dev/null
> +++ b/io/tst-bz26353.c
> @@ -0,0 +1,34 @@

It requires a proper Copyright header.

> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <ftw.h>
> +#include <sys/stat.h>
> +#include <errno.h>
> +#include <limits.h>
> +
> +int my_func(const char *file, const struct stat *sb ,int flag)
> +{
> +  printf ("%s\n", file);
> +  return 0;
> +}

This does not follow the glibc code style and conventions [1], it also
applied to the rest of the file.

Also please use the libsupport when creating new tests, check both
support/README and support/README-testing.c on how to use it.  You might
grep the tests include 'support/test-driver.c' to check for more examples.

> +
> +/*Check whether stack overflow occurs*/
> +int do_test(int large_nopenfd)
> +{
> +  int ret = ftw("./tst-bz26353", my_func, large_nopenfd);
> +  printf ("test big num %d, ret=%d errno=%d, without stack overflow\n",
> +          large_nopenfd, ret, errno);
> +  return 0;
> +}

You can check by using a subprocess instead of execve a new one,
check support_capture_subprocess on to use libsupport for test it.

If you do need to a run a different program, you might use
support_capture_subprogram; but I think this test do not really require
it.

> +
> +int main(int argc, char *argv[])
> +{
> +  int ret = 0;
> +  mkdir ("./tst-bz26353", 0755);

We have support_create_temp_directory to create temporary directories
that are remove on test exit.

> +  /* The default stack size is 8 MB.
> +   * Before the bug is fixed, stack overflow will be triggered. */
> +  ret += do_test(8192 * 1024);
> +  ret += do_test(INT_MAX);

This test will easily trash a low memory system (it allocate about 16GB
on my machine before I killed the process). 

> +  rmdir ("./tst-bz26353");
> +  return ret;
> +}
> 

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions