Message ID | 20200808084640.49174-1-nixiaoming@huawei.com |
---|---|
State | New |
Headers | show |
Series | io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353] | expand |
ping On 2020/8/8 16:46, 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 > > To fix the problem: > 1. Set the upper limit of descriptors to getdtablesize(). > 2. Replace alloca() in ftw_startup() with malloc(). > --- > io/Makefile | 3 ++- > io/ftw.c | 19 +++++++++++++++++-- > io/tst-bz26353.c | 20 ++++++++++++++++++++ > 3 files changed, 39 insertions(+), 3 deletions(-) > create mode 100644 io/tst-bz26353.c > > diff --git a/io/Makefile b/io/Makefile > index cf380f3516..0f674c317f 100644 > --- a/io/Makefile > +++ b/io/Makefile > @@ -74,7 +74,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 8c79d29a9e..094aada50c 100644 > --- a/io/ftw.c > +++ b/io/ftw.c > @@ -643,18 +643,32 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, > __set_errno (ENOENT); > return -1; > } > + if (descriptors > getdtablesize()) > + { > + __set_errno (EINVAL); > + return -1; > + } > > data.maxdir = descriptors < 1 ? 1 : descriptors; > data.actdir = 0; > - data.dirstreams = (struct dir_data **) alloca (data.maxdir > + data.dirstreams = (struct dir_data **) malloc (data.maxdir > * sizeof (struct dir_data *)); > + if (data.dirstreams == NULL) > + { > + __set_errno (ENOMEM); > + return -1; > + } > 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) > - return -1; > + { > + free (data.dirstreams); > + __set_errno (ENOMEM); > + return -1; > + } > cp = __stpcpy (data.dirbuf, dir); > /* Strip trailing slashes. */ > while (cp > data.dirbuf + 1 && cp[-1] == '/') > @@ -805,6 +819,7 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, > __tdestroy (data.known_objects, free); > free (data.dirbuf); > __set_errno (save_err); > + free (data.dirstreams); > > return result; > } > diff --git a/io/tst-bz26353.c b/io/tst-bz26353.c > new file mode 100644 > index 0000000000..4ab3d4b61f > --- /dev/null > +++ b/io/tst-bz26353.c > @@ -0,0 +1,20 @@ > +#include <stdlib.h> > +#include <stdio.h> > +#include <unistd.h> > +#include <ftw.h> > +#include <sys/stat.h> > + > +int my_func(const char *file , const struct stat *sb ,int flag) > +{ > + printf("%s\n", file); > + return 0; > +} > + > +int main(int argc, char *argv[]) > +{ > + mkdir("./tst-bz26353", 0755); > + /*Check whether stack overflow occurs*/ > + ftw("./tst-bz26353", my_func, 8192*1024); > + rmdir("./tst-bz26353"); > + return 0; > +} >
The patch isn't complete, since it doesn't check for integer overflow when multiplying data.maxdir by sizeof (struct dir_data *), where the function should also fail with errno == ENOMEM. You can check for overflow via intprops.h's INT_MULTIPLY_WRAPV (data.maxdir, sizeof (struct dir_data *)), &x) where x is of type size_t. Also, doesn't 'free' preserve errno? (If not, it should.) Then you need not set errno after malloc fails; the only time you need to set errno is when INT_MULTIPLY_WRAPV reports an overflow. The test case should have a test that uses INT_MAX.
On 2020/8/14 4:32, Paul Eggert wrote: > The patch isn't complete, since it doesn't check for integer overflow > when multiplying data.maxdir by sizeof (struct dir_data *), where the > function should also fail with errno == ENOMEM. You can check for > overflow via intprops.h's INT_MULTIPLY_WRAPV (data.maxdir, sizeof > (struct dir_data *)), &x) where x is of type size_t. > diff --git a/io/ftw.c b/io/ftw.c index 8c79d29a9e..094aada50c 100644 --- a/io/ftw.c +++ b/io/ftw.c @@ -643,18 +643,32 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, __set_errno (ENOENT); return -1; } + if (descriptors > getdtablesize()) + { + __set_errno (EINVAL); + return -1; + } linux/include/uapi/linux/fs.h:38:#define INR_OPEN_MAX 4096 /* Hard limit for nfile rlimits */ When data.maxdir is less than getdtablesize(), is there still a possibility that integer overflow occurs in data.maxdir * sizeof (struct dir_data *)? > Also, doesn't 'free' preserve errno? (If not, it should.) Then you need > not set errno after malloc fails; the only time you need to set errno is > when INT_MULTIPLY_WRAPV reports an overflow. Yes, here is my mistake, the malloc function itself has set the error code in the failure branch. > > The test case should have a test that uses INT_MAX. Yes, I missed the EINVAL branch test. Thank you. I'll send the v2 patch later.
On 2020/8/14 19:14, Xiaoming Ni wrote: > On 2020/8/14 4:32, Paul Eggert wrote: >> The patch isn't complete, since it doesn't check for integer overflow >> when multiplying data.maxdir by sizeof (struct dir_data *), where the >> function should also fail with errno == ENOMEM. You can check for >> overflow via intprops.h's INT_MULTIPLY_WRAPV (data.maxdir, sizeof >> (struct dir_data *)), &x) where x is of type size_t. >> > diff --git a/io/ftw.c b/io/ftw.c > index 8c79d29a9e..094aada50c 100644 > --- a/io/ftw.c > +++ b/io/ftw.c > @@ -643,18 +643,32 @@ ftw_startup (const char *dir, int is_nftw, void > *func, int descriptors, > __set_errno (ENOENT); > return -1; > } > + if (descriptors > getdtablesize()) > + { > + __set_errno (EINVAL); > + return -1; > + } > linux/include/uapi/linux/fs.h:38:#define INR_OPEN_MAX 4096 /* Hard > limit for nfile rlimits */ > > When data.maxdir is less than getdtablesize(), is there still a > possibility that integer overflow occurs in data.maxdir * sizeof (struct > dir_data *)? > on linux: The maximum number of process file handles is sysctl_nr_open. /proc/sys/fs/nr_open The maximum value is sysctl_nr_open_max. fs/file.c : #define __const_min(x, y) ((x) < (y)? (x): (y)) unsigned int sysctl_nr_open_max = __const_min(INT_MAX, ~(size_t)0/sizeof(void *)) & -BITS_PER_LONG; On a 32 - bit machine, BITS_PER_LONG is 32 INT_MAX is 0x7fffffff SIZE_MAX is 0xffffffff sysctl_nr_open_max is 0x3fffffe0 sysctl_nr_open_max * sizeof (struct dir_data *)) is 0xffffff80 The value is greater than INT_MAX but less than SIZE_MAX. No overflow occurs. On a 64-bit machine BITS_PER_LONG is 64 INT_MAX is 0x7fffffff SIZE_MAX is 0xffffffffffffffff sysctl_nr_open_max is 0x7fffffc0 sysctl_nr_open_max * sizeof (struct dir_data *)) is 0x3ffffff00 The value is greater than INT_MAX but less than SIZE_MAX. No overflow occurs. Thanks Xiaoming Ni
diff --git a/io/Makefile b/io/Makefile index cf380f3516..0f674c317f 100644 --- a/io/Makefile +++ b/io/Makefile @@ -74,7 +74,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 8c79d29a9e..094aada50c 100644 --- a/io/ftw.c +++ b/io/ftw.c @@ -643,18 +643,32 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, __set_errno (ENOENT); return -1; } + if (descriptors > getdtablesize()) + { + __set_errno (EINVAL); + return -1; + } data.maxdir = descriptors < 1 ? 1 : descriptors; data.actdir = 0; - data.dirstreams = (struct dir_data **) alloca (data.maxdir + data.dirstreams = (struct dir_data **) malloc (data.maxdir * sizeof (struct dir_data *)); + if (data.dirstreams == NULL) + { + __set_errno (ENOMEM); + return -1; + } 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) - return -1; + { + free (data.dirstreams); + __set_errno (ENOMEM); + return -1; + } cp = __stpcpy (data.dirbuf, dir); /* Strip trailing slashes. */ while (cp > data.dirbuf + 1 && cp[-1] == '/') @@ -805,6 +819,7 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, __tdestroy (data.known_objects, free); free (data.dirbuf); __set_errno (save_err); + free (data.dirstreams); return result; } diff --git a/io/tst-bz26353.c b/io/tst-bz26353.c new file mode 100644 index 0000000000..4ab3d4b61f --- /dev/null +++ b/io/tst-bz26353.c @@ -0,0 +1,20 @@ +#include <stdlib.h> +#include <stdio.h> +#include <unistd.h> +#include <ftw.h> +#include <sys/stat.h> + +int my_func(const char *file , const struct stat *sb ,int flag) +{ + printf("%s\n", file); + return 0; +} + +int main(int argc, char *argv[]) +{ + mkdir("./tst-bz26353", 0755); + /*Check whether stack overflow occurs*/ + ftw("./tst-bz26353", my_func, 8192*1024); + rmdir("./tst-bz26353"); + return 0; +}