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

Message ID 20200808084640.49174-1-nixiaoming@huawei.com
State New
Headers show
Series
  • io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
Related show

Commit Message

Xiaoming Ni Aug. 8, 2020, 8:46 a.m.
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

-- 
2.27.0

Comments

Xiaoming Ni Aug. 13, 2020, 3:26 a.m. | #1
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;

> +}

>
Paul Eggert Aug. 13, 2020, 8:32 p.m. | #2
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.
Xiaoming Ni Aug. 14, 2020, 11:14 a.m. | #3
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.
Xiaoming Ni Aug. 16, 2020, 8:53 a.m. | #4
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

Patch

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