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

  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
  

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