[PATCH v4] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
Xiaoming Ni
nixiaoming@huawei.com
Thu Nov 26 02:21:26 GMT 2020
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.
> .
>
More information about the Libc-alpha
mailing list