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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 26/11/2020 00: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.
> 
> -------------------
> v5:
>   Based on Adhemerval Zanella's review comments,
>   Use TEST_COMPARE to replace some code for checking return values in testcase.
> 
> v4: https://public-inbox.org/libc-alpha/20201125100659.30923-1-nixiaoming@huawei.com/
>   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/

LGTM, I have pushed it with some minor changes below.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  io/Makefile      |  3 +-
>  io/ftw.c         | 16 +++++-----
>  io/tst-bz26353.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 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
>  

I renamed the new test to tst-ftw-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..2730da4cfb
> --- /dev/null
> +++ b/io/tst-bz26353.c
> @@ -0,0 +1,76 @@
> +/* 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 int
> +get_large_nopenfd_by_RLIMIT_STACK (void)
> +{
> +  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);
> +    }
> +  return (int) r.rlim_cur;
> +}
> +
> +static void
> +do_ftw (void *unused)
> +{
> +  char *tempdir = support_create_temp_directory ("tst-bz26353");
> +  int large_nopenfd = get_large_nopenfd_by_RLIMIT_STACK ();
> +  int ret = ftw (tempdir, my_func, large_nopenfd);
> +  if (ret != 0)
> +    {
> +      printf ("ftw() fail but no stack overflow: nopenfd=%d ret=%d errno=%d\n",
> +              large_nopenfd, ret, errno);
> +    }

I changed to:

  TEST_COMPARE (ftw (tempdir, my_func, large_nopenfd), 0);

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