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

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

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