* [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
@ 2020-08-07 10:16 Xiaoming Ni
2020-08-07 19:43 ` Adhemerval Zanella
2020-08-07 23:56 ` Paul Eggert
0 siblings, 2 replies; 11+ messages in thread
From: Xiaoming Ni @ 2020-08-07 10:16 UTC (permalink / raw)
To: libc-alpha, glibc-bugs, unassigned, drepper.fsp, roland, carlos,
adhemerval.zanella
Cc: nixiaoming, wangle6, yukeji
Realpath() cyclically invokes __alloca() when processing soft link files,
which may consume 164 KB stack space.
Therefore, replace __alloca with malloc to reduce stack overflow risks
v2: Avoid repeated malloc and free operations. and add testcase
v1: https://patches-gcc.linaro.org/patch/39851/
Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
stdlib/Makefile | 3 +-
stdlib/canonicalize.c | 25 +++++++++++++--
stdlib/tst-bz26341.c | 73 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 98 insertions(+), 3 deletions(-)
create mode 100644 stdlib/tst-bz26341.c
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 4615f6dfe7..bfdd9036b1 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -87,7 +87,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
tst-makecontext-align test-bz22786 tst-strtod-nan-sign \
tst-swapcontext1 tst-setcontext4 tst-setcontext5 \
tst-setcontext6 tst-setcontext7 tst-setcontext8 \
- tst-setcontext9 tst-bz20544
+ tst-setcontext9 tst-bz20544 tst-bz26341
tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
tst-tls-atexit tst-tls-atexit-nodelete
@@ -98,6 +98,7 @@ ifeq ($(build-hardcoded-path-in-tests),yes)
tests += tst-empty-env
endif
+LDLIBS-tst-bz26341 = -lpthread
LDLIBS-test-atexit-race = $(shared-thread-library)
LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index cbd885a3c5..c02a8a5800 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -46,6 +46,7 @@ __realpath (const char *name, char *resolved)
const char *start, *end, *rpath_limit;
long int path_max;
int num_links = 0;
+ char *buf = NULL;
if (name == NULL)
{
@@ -163,9 +164,18 @@ __realpath (const char *name, char *resolved)
if (S_ISLNK (st.st_mode))
{
- char *buf = __alloca (path_max);
size_t len;
+ if (buf == NULL)
+ {
+ buf = malloc (path_max);
+ if (buf == NULL)
+ {
+ __set_errno (ENOMEM);
+ goto error;
+ }
+ }
+
if (++num_links > __eloop_threshold ())
{
__set_errno (ELOOP);
@@ -178,7 +188,14 @@ __realpath (const char *name, char *resolved)
buf[n] = '\0';
if (!extra_buf)
- extra_buf = __alloca (path_max);
+ {
+ extra_buf = malloc (path_max);
+ if (extra_buf == NULL)
+ {
+ __set_errno (ENOMEM);
+ goto error;
+ }
+ }
len = strlen (end);
if (path_max - n <= len)
@@ -210,12 +227,16 @@ __realpath (const char *name, char *resolved)
*dest = '\0';
assert (resolved == NULL || resolved == rpath);
+ free(extra_buf);
+ free(buf);
return rpath;
error:
assert (resolved == NULL || resolved == rpath);
if (resolved == NULL)
free (rpath);
+ free (extra_buf);
+ free (buf);
return NULL;
}
libc_hidden_def (__realpath)
diff --git a/stdlib/tst-bz26341.c b/stdlib/tst-bz26341.c
new file mode 100644
index 0000000000..0fe095b7d1
--- /dev/null
+++ b/stdlib/tst-bz26341.c
@@ -0,0 +1,73 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <limits.h>
+#include <sys/resource.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <string.h>
+#include <pthread.h>
+
+int creat_link(void)
+{
+ int i;
+ int fd;
+ char fname[2][16] = {0};
+ char *p1 = fname[0];
+ char *p2 = fname[1];
+ strcpy(p1, "f0");
+ fd = open(p1, O_RDONLY|O_CREAT, 0444);
+ close(fd);
+
+ for (i = 0; i < 41; i++) {
+ sprintf(p2, "f%d", i);
+ symlink(p1, p2);
+ p1 = p2;
+ p2 = fname[i % 2];
+ }
+ return 0;
+}
+
+void clean_link(void)
+{
+ char fname[16] = {0};
+ int i;
+ for (i = 0; i < 41; i++) {
+ sprintf(fname, "f%d", i);
+ unlink(fname);
+ }
+}
+
+void *do_realpath(void *ignore)
+{
+ char *p = realpath("f40", NULL);
+ printf("%p\n", p);
+ if (p != NULL)
+ printf("%s\n", p);
+ return NULL;
+}
+
+/* Set different stack sizes and check whether stack overflow occurs. */
+int do_test(int size)
+{
+ pthread_t tid;
+ pthread_attr_t thread_attr;
+ pthread_attr_init(&thread_attr);
+ pthread_attr_setstacksize(&thread_attr, size);
+
+ pthread_create(&tid, &thread_attr, do_realpath, NULL);
+ pthread_join(tid, NULL);
+ return 0;
+}
+
+int main(int argc, char *argv[])
+{
+ creat_link();
+ do_test(8192*1024);
+ do_test(160*1024);
+ do_test(40*1024);
+ clean_link();
+ printf("\n");
+ return 0;
+}
+
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-07 10:16 [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341] Xiaoming Ni
@ 2020-08-07 19:43 ` Adhemerval Zanella
2020-08-08 0:00 ` Paul Eggert
2020-08-08 9:14 ` Xiaoming Ni
2020-08-07 23:56 ` Paul Eggert
1 sibling, 2 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2020-08-07 19:43 UTC (permalink / raw)
To: Xiaoming Ni, libc-alpha, glibc-bugs, unassigned, drepper.fsp,
roland, carlos
Cc: wangle6, yukeji, Paul Eggert
On 07/08/2020 07:16, Xiaoming Ni wrote:
> Realpath() cyclically invokes __alloca() when processing soft link files,
> which may consume 164 KB stack space.
> Therefore, replace __alloca with malloc to reduce stack overflow risks
>
> v2: Avoid repeated malloc and free operations. and add testcase
> v1: https://patches-gcc.linaro.org/patch/39851/
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
Again, we do not use DCO, but rather Copyright assignment.
Paul, it might be something to be fixed on gnulib since I noted that both
gpl and lgpl code uses malloca (which calls alloca if the header is present).
Some comments below regarding testing.
> ---
> stdlib/Makefile | 3 +-
> stdlib/canonicalize.c | 25 +++++++++++++--
> stdlib/tst-bz26341.c | 73 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 98 insertions(+), 3 deletions(-)
> create mode 100644 stdlib/tst-bz26341.c
>
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 4615f6dfe7..bfdd9036b1 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -87,7 +87,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
> tst-makecontext-align test-bz22786 tst-strtod-nan-sign \
> tst-swapcontext1 tst-setcontext4 tst-setcontext5 \
> tst-setcontext6 tst-setcontext7 tst-setcontext8 \
> - tst-setcontext9 tst-bz20544
> + tst-setcontext9 tst-bz20544 tst-bz26341
>
> tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
> tst-tls-atexit tst-tls-atexit-nodelete
Ok.
> @@ -98,6 +98,7 @@ ifeq ($(build-hardcoded-path-in-tests),yes)
> tests += tst-empty-env
> endif
>
> +LDLIBS-tst-bz26341 = -lpthread
This needs to be $(shared-thread-library).
> LDLIBS-test-atexit-race = $(shared-thread-library)
> LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
> LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
> diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
> index cbd885a3c5..c02a8a5800 100644
> --- a/stdlib/canonicalize.c
> +++ b/stdlib/canonicalize.c
> @@ -46,6 +46,7 @@ __realpath (const char *name, char *resolved)
> const char *start, *end, *rpath_limit;
> long int path_max;
> int num_links = 0;
> + char *buf = NULL;
>
> if (name == NULL)
> {
Ok.
> @@ -163,9 +164,18 @@ __realpath (const char *name, char *resolved)
>
> if (S_ISLNK (st.st_mode))
> {
> - char *buf = __alloca (path_max);
> size_t len;
>
> + if (buf == NULL)
> + {
> + buf = malloc (path_max);
> + if (buf == NULL)
> + {
> + __set_errno (ENOMEM);
> + goto error;
> + }
> + }
> +
> if (++num_links > __eloop_threshold ())
> {
> __set_errno (ELOOP);
Ok.
> @@ -178,7 +188,14 @@ __realpath (const char *name, char *resolved)
> buf[n] = '\0';
>
> if (!extra_buf)
> - extra_buf = __alloca (path_max);
> + {
> + extra_buf = malloc (path_max);
> + if (extra_buf == NULL)
> + {
> + __set_errno (ENOMEM);
> + goto error;
> + }
> + }
>
> len = strlen (end);
> if (path_max - n <= len)
Ok.
> @@ -210,12 +227,16 @@ __realpath (const char *name, char *resolved)
> *dest = '\0';
>
> assert (resolved == NULL || resolved == rpath);
> + free(extra_buf);
> + free(buf);
> return rpath;
>
> error:
> assert (resolved == NULL || resolved == rpath);
> if (resolved == NULL)
> free (rpath);
> + free (extra_buf);
> + free (buf);
> return NULL;
> }
> libc_hidden_def (__realpath)
Ok.
> diff --git a/stdlib/tst-bz26341.c b/stdlib/tst-bz26341.c
> new file mode 100644
> index 0000000000..0fe095b7d1
> --- /dev/null
> +++ b/stdlib/tst-bz26341.c
> @@ -0,0 +1,73 @@
This test need the Copyright header and to be properly indented using glibc
code guideline.
Use it need to use the libsupport (check support/README-testing.c and other
tests that use '#include <support/test-driver.c>'.
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <limits.h>
> +#include <sys/resource.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <pthread.h>
> +
> +int creat_link(void)
> +{
> + int i;
> + int fd;
> + char fname[2][16] = {0};
> + char *p1 = fname[0];
> + char *p2 = fname[1];
> + strcpy(p1, "f0");
> + fd = open(p1, O_RDONLY|O_CREAT, 0444);
> + close(fd);
> +
> + for (i = 0; i < 41; i++) {
> + sprintf(p2, "f%d", i);
> + symlink(p1, p2);
> + p1 = p2;
> + p2 = fname[i % 2];
> + }
> + return 0;
> +}
> +
> +void clean_link(void)
> +{
> + char fname[16] = {0};
> + int i;
> + for (i = 0; i < 41; i++) {
> + sprintf(fname, "f%d", i);
> + unlink(fname);
> + }
> +}
> +
> +void *do_realpath(void *ignore)
> +{
> + char *p = realpath("f40", NULL);
> + printf("%p\n", p);
> + if (p != NULL)
> + printf("%s\n", p);
> + return NULL;
> +}
> +
> +/* Set different stack sizes and check whether stack overflow occurs. */
> +int do_test(int size)
> +{
> + pthread_t tid;
> + pthread_attr_t thread_attr;
> + pthread_attr_init(&thread_attr);
> + pthread_attr_setstacksize(&thread_attr, size);
> +
> + pthread_create(&tid, &thread_attr, do_realpath, NULL);
> + pthread_join(tid, NULL);
> + return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + creat_link();
> + do_test(8192*1024);
> + do_test(160*1024);
> + do_test(40*1024);
It think it would be suffice to just check with a small stacksize that triggers
the failure on current code.
> + clean_link();
> + printf("\n");
> + return 0;
> +}
> +
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-07 10:16 [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341] Xiaoming Ni
2020-08-07 19:43 ` Adhemerval Zanella
@ 2020-08-07 23:56 ` Paul Eggert
2020-08-08 8:54 ` Xiaoming Ni
1 sibling, 1 reply; 11+ messages in thread
From: Paul Eggert @ 2020-08-07 23:56 UTC (permalink / raw)
To: Xiaoming Ni, libc-alpha, glibc-bugs, unassigned, drepper.fsp,
roland, carlos, adhemerval.zanella
Cc: yukeji, wangle6
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
On 8/7/20 3:16 AM, Xiaoming Ni wrote:
> Realpath() cyclically invokes __alloca() when processing soft link files,
> which may consume 164 KB stack space.
> Therefore, replace __alloca with malloc to reduce stack overflow risks
I don't understand why malloc is required here.
Isn't the problem that the buffer is being allocated over and over again? And if
you fix that bug, the function should allocate only 8 KiB via __alloca on
GNU/Linux. Something like the attached (untested) patch, say. So why not keep
using the stack? That would be more efficient than resorting to the heap.
[-- Attachment #2: realpath.diff --]
[-- Type: text/x-patch, Size: 850 bytes --]
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index cbd885a3c5..ddeea3e2e9 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -42,7 +42,7 @@
char *
__realpath (const char *name, char *resolved)
{
- char *rpath, *dest, *extra_buf = NULL;
+ char *rpath, *dest, *buf = NULL, *extra_buf = NULL;
const char *start, *end, *rpath_limit;
long int path_max;
int num_links = 0;
@@ -163,7 +163,6 @@ __realpath (const char *name, char *resolved)
if (S_ISLNK (st.st_mode))
{
- char *buf = __alloca (path_max);
size_t len;
if (++num_links > __eloop_threshold ())
@@ -172,6 +171,8 @@ __realpath (const char *name, char *resolved)
goto error;
}
+ if (!buf)
+ buf = __alloca (path_max);
n = __readlink (rpath, buf, path_max - 1);
if (n < 0)
goto error;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-07 19:43 ` Adhemerval Zanella
@ 2020-08-08 0:00 ` Paul Eggert
2020-08-08 9:14 ` Xiaoming Ni
1 sibling, 0 replies; 11+ messages in thread
From: Paul Eggert @ 2020-08-08 0:00 UTC (permalink / raw)
To: Adhemerval Zanella, Xiaoming Ni, libc-alpha, glibc-bugs,
unassigned, drepper.fsp, roland, carlos
Cc: wangle6, yukeji
On 8/7/20 12:43 PM, Adhemerval Zanella wrote:
> Paul, it might be something to be fixed on gnulib since I noted that both
> gpl and lgpl code uses malloca (which calls alloca if the header is present).
The Gnulib canonicalize.c is a quite-different implementation, and does not use
alloca.
There is quite a bit of Gnulib code that does use alloca, but it is supposed to
do so without allocating and using more than 4032 bytes at a time, which is safe
on all the Gnulib targets we know about.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-07 23:56 ` Paul Eggert
@ 2020-08-08 8:54 ` Xiaoming Ni
2020-08-09 8:44 ` Paul Eggert
0 siblings, 1 reply; 11+ messages in thread
From: Xiaoming Ni @ 2020-08-08 8:54 UTC (permalink / raw)
To: Paul Eggert, libc-alpha, glibc-bugs, unassigned, drepper.fsp,
roland, carlos, adhemerval.zanella
Cc: yukeji, wangle6
On 2020/8/8 7:56, Paul Eggert wrote:
> I don't understand why malloc is required here.
>
> Isn't the problem that the buffer is being allocated over and over
> again? And if you fix that bug, the function should allocate only 8 KiB
> via __alloca on GNU/Linux. Something like the attached (untested) patch,
> say. So why not keep using the stack? That would be more efficient than
> resorting to the heap.
To avoid possible stack overflow risks (the remaining stack space is
uncertain when realpath is called), should we sacrifice some efficiency
and reduce the stack space usage? Use malloc instead of alloca (4k+4k).
thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-07 19:43 ` Adhemerval Zanella
2020-08-08 0:00 ` Paul Eggert
@ 2020-08-08 9:14 ` Xiaoming Ni
1 sibling, 0 replies; 11+ messages in thread
From: Xiaoming Ni @ 2020-08-08 9:14 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha, glibc-bugs, unassigned,
drepper.fsp, roland, carlos
Cc: wangle6, yukeji, Paul Eggert
On 2020/8/8 3:43, Adhemerval Zanella wrote:
>
>
> On 07/08/2020 07:16, Xiaoming Ni wrote:
>> Realpath() cyclically invokes __alloca() when processing soft link files,
>> which may consume 164 KB stack space.
>> Therefore, replace __alloca with malloc to reduce stack overflow risks
>>
>> v2: Avoid repeated malloc and free operations. and add testcase
>> v1: https://patches-gcc.linaro.org/patch/39851/
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>
> Again, we do not use DCO, but rather Copyright assignment.
Do I just need to delete this line: "Signed-off-by: Xiaoming Ni
<nixiaoming@huawei.com>"?
Does this mean that glibc doesn't use
Signed-off-by/Reported-by/Tested-by/Reviewed-by?
> Paul, it might be something to be fixed on gnulib since I noted that both
> gpl and lgpl code uses malloca (which calls alloca if the header is present).
>
> Some comments below regarding testing.
>
>> ---
>> stdlib/Makefile | 3 +-
>> stdlib/canonicalize.c | 25 +++++++++++++--
>> stdlib/tst-bz26341.c | 73 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 98 insertions(+), 3 deletions(-)
>> create mode 100644 stdlib/tst-bz26341.c
>>
>> diff --git a/stdlib/Makefile b/stdlib/Makefile
>> index 4615f6dfe7..bfdd9036b1 100644
>> --- a/stdlib/Makefile
>> +++ b/stdlib/Makefile
>> @@ -87,7 +87,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
>> tst-makecontext-align test-bz22786 tst-strtod-nan-sign \
>> tst-swapcontext1 tst-setcontext4 tst-setcontext5 \
>> tst-setcontext6 tst-setcontext7 tst-setcontext8 \
>> - tst-setcontext9 tst-bz20544
>> + tst-setcontext9 tst-bz20544 tst-bz26341
>>
>> tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
>> tst-tls-atexit tst-tls-atexit-nodelete
>
> Ok.
>
>> @@ -98,6 +98,7 @@ ifeq ($(build-hardcoded-path-in-tests),yes)
>> tests += tst-empty-env
>> endif
>>
>> +LDLIBS-tst-bz26341 = -lpthread
>
> This needs to be $(shared-thread-library).
>
>> LDLIBS-test-atexit-race = $(shared-thread-library)
>> LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
>> LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
>> diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
>> index cbd885a3c5..c02a8a5800 100644
>> --- a/stdlib/canonicalize.c
>> +++ b/stdlib/canonicalize.c
>> @@ -46,6 +46,7 @@ __realpath (const char *name, char *resolved)
>> const char *start, *end, *rpath_limit;
>> long int path_max;
>> int num_links = 0;
>> + char *buf = NULL;
>>
>> if (name == NULL)
>> {
>
> Ok.
>
>> @@ -163,9 +164,18 @@ __realpath (const char *name, char *resolved)
>>
>> if (S_ISLNK (st.st_mode))
>> {
>> - char *buf = __alloca (path_max);
>> size_t len;
>>
>> + if (buf == NULL)
>> + {
>> + buf = malloc (path_max);
>> + if (buf == NULL)
>> + {
>> + __set_errno (ENOMEM);
>> + goto error;
>> + }
>> + }
>> +
>> if (++num_links > __eloop_threshold ())
>> {
>> __set_errno (ELOOP);
>
> Ok.
>
>> @@ -178,7 +188,14 @@ __realpath (const char *name, char *resolved)
>> buf[n] = '\0';
>>
>> if (!extra_buf)
>> - extra_buf = __alloca (path_max);
>> + {
>> + extra_buf = malloc (path_max);
>> + if (extra_buf == NULL)
>> + {
>> + __set_errno (ENOMEM);
>> + goto error;
>> + }
>> + }
>>
>> len = strlen (end);
>> if (path_max - n <= len)
>
> Ok.
>
>> @@ -210,12 +227,16 @@ __realpath (const char *name, char *resolved)
>> *dest = '\0';
>>
>> assert (resolved == NULL || resolved == rpath);
>> + free(extra_buf);
>> + free(buf);
>> return rpath;
>>
>> error:
>> assert (resolved == NULL || resolved == rpath);
>> if (resolved == NULL)
>> free (rpath);
>> + free (extra_buf);
>> + free (buf);
>> return NULL;
>> }
>> libc_hidden_def (__realpath)
>
> Ok.
>
>> diff --git a/stdlib/tst-bz26341.c b/stdlib/tst-bz26341.c
>> new file mode 100644
>> index 0000000000..0fe095b7d1
>> --- /dev/null
>> +++ b/stdlib/tst-bz26341.c
>> @@ -0,0 +1,73 @@
>
> This test need the Copyright header and to be properly indented using glibc
> code guideline.
>
Is that it?
/* 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/>. */
> Use it need to use the libsupport (check support/README-testing.c and other
> tests that use '#include <support/test-driver.c>'.
sorry, I'm not familiar with the glibc test suite
If the test case is successful, 0 is returned. If the test case fails, a
non-zero value is returned. Is this sufficient?
I see that the main() function is also used in tst-qsort.c.
Is support/test-driver.c mandatory?
thanks
Xiaoming Ni
>
>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <limits.h>
>> +#include <sys/resource.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +#include <string.h>
>> +#include <pthread.h>
>> +
>> +int creat_link(void)
>> +{
>> + int i;
>> + int fd;
>> + char fname[2][16] = {0};
>> + char *p1 = fname[0];
>> + char *p2 = fname[1];
>> + strcpy(p1, "f0");
>> + fd = open(p1, O_RDONLY|O_CREAT, 0444);
>> + close(fd);
>> +
>> + for (i = 0; i < 41; i++) {
>> + sprintf(p2, "f%d", i);
>> + symlink(p1, p2);
>> + p1 = p2;
>> + p2 = fname[i % 2];
>> + }
>> + return 0;
>> +}
>> +
>> +void clean_link(void)
>> +{
>> + char fname[16] = {0};
>> + int i;
>> + for (i = 0; i < 41; i++) {
>> + sprintf(fname, "f%d", i);
>> + unlink(fname);
>> + }
>> +}
>> +
>> +void *do_realpath(void *ignore)
>> +{
>> + char *p = realpath("f40", NULL);
>> + printf("%p\n", p);
>> + if (p != NULL)
>> + printf("%s\n", p);
>> + return NULL;
>> +}
>> +
>> +/* Set different stack sizes and check whether stack overflow occurs. */
>> +int do_test(int size)
>> +{
>> + pthread_t tid;
>> + pthread_attr_t thread_attr;
>> + pthread_attr_init(&thread_attr);
>> + pthread_attr_setstacksize(&thread_attr, size);
>> +
>> + pthread_create(&tid, &thread_attr, do_realpath, NULL);
>> + pthread_join(tid, NULL);
>> + return 0;
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + creat_link();
>> + do_test(8192*1024);
>> + do_test(160*1024);
>> + do_test(40*1024);
>
> It think it would be suffice to just check with a small stacksize that triggers
> the failure on current code.
Yes, you're right.
>
>> + clean_link();
>> + printf("\n");
>> + return 0;
>> +}
>> +
>>
>
> .
>
Thanks
Xiaoming Ni
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-08 8:54 ` Xiaoming Ni
@ 2020-08-09 8:44 ` Paul Eggert
2020-08-09 12:38 ` Florian Weimer
0 siblings, 1 reply; 11+ messages in thread
From: Paul Eggert @ 2020-08-09 8:44 UTC (permalink / raw)
To: Xiaoming Ni, libc-alpha, glibc-bugs, unassigned, drepper.fsp,
roland, carlos, adhemerval.zanella
Cc: yukeji, wangle6
On 8/8/20 1:54 AM, Xiaoming Ni wrote:
> To avoid possible stack overflow risks (the remaining stack space is uncertain
> when realpath is called), should we sacrifice some efficiency and reduce the
> stack space usage? Use malloc instead of alloca (4k+4k).
8 KiB of stack in a non-recursive I/O routine is not that big a deal. I don't
see why we would need to worry about that.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-09 8:44 ` Paul Eggert
@ 2020-08-09 12:38 ` Florian Weimer
2020-08-09 17:22 ` Paul Eggert
0 siblings, 1 reply; 11+ messages in thread
From: Florian Weimer @ 2020-08-09 12:38 UTC (permalink / raw)
To: Paul Eggert
Cc: Xiaoming Ni, libc-alpha, drepper.fsp, roland, carlos,
adhemerval.zanella, wangle6, yukeji
* Paul Eggert:
> On 8/8/20 1:54 AM, Xiaoming Ni wrote:
>> To avoid possible stack overflow risks (the remaining stack space is uncertain
>> when realpath is called), should we sacrifice some efficiency and reduce the
>> stack space usage? Use malloc instead of alloca (4k+4k).
>
> 8 KiB of stack in a non-recursive I/O routine is not that big a deal. I don't
> see why we would need to worry about that.
I have seen a report that the temporary buffer in vfprintf on an
unbuffered stream causes crashes because after a hardware upgrade, the
available stack space was insufficient. That on-stack buffer is 8 KiB
as well.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-09 12:38 ` Florian Weimer
@ 2020-08-09 17:22 ` Paul Eggert
2020-08-10 13:40 ` Adhemerval Zanella
0 siblings, 1 reply; 11+ messages in thread
From: Paul Eggert @ 2020-08-09 17:22 UTC (permalink / raw)
To: Florian Weimer
Cc: Xiaoming Ni, libc-alpha, drepper.fsp, roland, carlos,
adhemerval.zanella, wangle6, yukeji
On 8/9/20 5:38 AM, Florian Weimer wrote:
> I have seen a report that the temporary buffer in vfprintf on an
> unbuffered stream causes crashes because after a hardware upgrade, the
> available stack space was insufficient. That on-stack buffer is 8 KiB
> as well.
I have no doubt that there are more bug reports about stack overflows "caused"
by vfprintf's 8 KiB stack than about those "caused" by getchar's 2 KiB stack.
But this doesn't mean we should worry overmuch about these small stack
allocations. It's a fact of life that library routines use a small amount of
stack space, and on today's processors 8 KiB in a leaf I/O function counts as
"small" even in multithreaded apps. We shouldn't waste valuable development time
(or user CPU time) trying to shrink such a function's stack space further.
It's easy to shrink 164 KiB (as in the original bug report) down to 8 KiB, so
let's do that. The need to shrink stack further does not outweigh the need to
avoid pressuring the memory allocator and worrying about leaks, so let's quit
while we're ahead.
If (despite my advice) there is a push to shrink the stack space below 8 KiB, at
the very least we should introduce no new call to malloc when all processing can
be done in a single buffer containing only PATH_MAX bytes (which describes the
overwhelming majority of real-world cases).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-09 17:22 ` Paul Eggert
@ 2020-08-10 13:40 ` Adhemerval Zanella
2020-08-11 9:54 ` Paul Eggert
0 siblings, 1 reply; 11+ messages in thread
From: Adhemerval Zanella @ 2020-08-10 13:40 UTC (permalink / raw)
To: Paul Eggert, Florian Weimer
Cc: Xiaoming Ni, libc-alpha, drepper.fsp, roland, carlos, wangle6, yukeji
On 09/08/2020 14:22, Paul Eggert wrote:
> On 8/9/20 5:38 AM, Florian Weimer wrote:
>> I have seen a report that the temporary buffer in vfprintf on an
>> unbuffered stream causes crashes because after a hardware upgrade, the
>> available stack space was insufficient. That on-stack buffer is 8 KiB
>> as well.
>
> I have no doubt that there are more bug reports about stack overflows "caused" by vfprintf's 8 KiB stack than about those "caused" by getchar's 2 KiB stack. But this doesn't mean we should worry overmuch about these small stack allocations. It's a fact of life that library routines use a small amount of stack space, and on today's processors 8 KiB in a leaf I/O function counts as "small" even in multithreaded apps. We shouldn't waste valuable development time (or user CPU time) trying to shrink such a function's stack space further.
>
> It's easy to shrink 164 KiB (as in the original bug report) down to 8 KiB, so let's do that. The need to shrink stack further does not outweigh the need to avoid pressuring the memory allocator and worrying about leaks, so let's quit while we're ahead.
>
> If (despite my advice) there is a push to shrink the stack space below 8 KiB, at the very least we should introduce no new call to malloc when all processing can be done in a single buffer containing only PATH_MAX bytes (which describes the overwhelming majority of real-world cases).
Could we remove alloca as well? We can make 4k stack usage on most cases use the
extra 4k only where link are seeing the path.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
2020-08-10 13:40 ` Adhemerval Zanella
@ 2020-08-11 9:54 ` Paul Eggert
0 siblings, 0 replies; 11+ messages in thread
From: Paul Eggert @ 2020-08-11 9:54 UTC (permalink / raw)
To: Adhemerval Zanella, Florian Weimer
Cc: Xiaoming Ni, libc-alpha, drepper.fsp, roland, carlos, wangle6, yukeji
On 8/10/20 6:40 AM, Adhemerval Zanella wrote:
> Could we remove alloca as well? We can make 4k stack usage on most cases use the
> extra 4k only where link are seeing the path.
Yes, the only "cost" of that is that the function will always use 4K (or
whatever) instead of typically using far less than 4K.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-08-11 9:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-07 10:16 [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341] Xiaoming Ni
2020-08-07 19:43 ` Adhemerval Zanella
2020-08-08 0:00 ` Paul Eggert
2020-08-08 9:14 ` Xiaoming Ni
2020-08-07 23:56 ` Paul Eggert
2020-08-08 8:54 ` Xiaoming Ni
2020-08-09 8:44 ` Paul Eggert
2020-08-09 12:38 ` Florian Weimer
2020-08-09 17:22 ` Paul Eggert
2020-08-10 13:40 ` Adhemerval Zanella
2020-08-11 9:54 ` Paul Eggert
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).