* [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
@ 2020-08-08 8:46 Xiaoming Ni
2020-08-13 3:26 ` ping // " Xiaoming Ni
2020-08-13 20:32 ` Paul Eggert
0 siblings, 2 replies; 5+ messages in thread
From: Xiaoming Ni @ 2020-08-08 8:46 UTC (permalink / raw)
To: libc-alpha, carlos, eggert, adhemerval.zanella, glibc-bugs,
unassigned, drepper.fsp
Cc: nixiaoming, wangle6
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;
+}
--
2.27.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* ping // [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
2020-08-08 8:46 [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353] Xiaoming Ni
@ 2020-08-13 3:26 ` Xiaoming Ni
2020-08-13 20:32 ` Paul Eggert
1 sibling, 0 replies; 5+ messages in thread
From: Xiaoming Ni @ 2020-08-13 3:26 UTC (permalink / raw)
To: libc-alpha, carlos, eggert, adhemerval.zanella, glibc-bugs,
unassigned, drepper.fsp
Cc: wangle6
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;
> +}
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
2020-08-08 8:46 [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353] Xiaoming Ni
2020-08-13 3:26 ` ping // " Xiaoming Ni
@ 2020-08-13 20:32 ` Paul Eggert
2020-08-14 11:14 ` Xiaoming Ni
1 sibling, 1 reply; 5+ messages in thread
From: Paul Eggert @ 2020-08-13 20:32 UTC (permalink / raw)
To: Xiaoming Ni; +Cc: libc-alpha
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
2020-08-13 20:32 ` Paul Eggert
@ 2020-08-14 11:14 ` Xiaoming Ni
2020-08-16 8:53 ` Xiaoming Ni
0 siblings, 1 reply; 5+ messages in thread
From: Xiaoming Ni @ 2020-08-14 11:14 UTC (permalink / raw)
To: Paul Eggert; +Cc: libc-alpha
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353]
2020-08-14 11:14 ` Xiaoming Ni
@ 2020-08-16 8:53 ` Xiaoming Ni
0 siblings, 0 replies; 5+ messages in thread
From: Xiaoming Ni @ 2020-08-16 8:53 UTC (permalink / raw)
To: Paul Eggert; +Cc: libc-alpha
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-16 8:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-08 8:46 [PATCH] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353] Xiaoming Ni
2020-08-13 3:26 ` ping // " Xiaoming Ni
2020-08-13 20:32 ` Paul Eggert
2020-08-14 11:14 ` Xiaoming Ni
2020-08-16 8:53 ` Xiaoming Ni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).