[v3,2/4,v2] stdlib: Use fixed buffer size for realpath [BZ #26241]
Commit Message
Changes from previous version [1]:
- I have abandoned the scratch_buffer usage, the PATH_MAX stack usage
is acceptable and it avoid the potentially malloc usage.
[1] https://sourceware.org/pipermail/libc-alpha/2020-August/117078.html
---
It uses both a fixed internal buffer with PATH_MAX size to read and
copy the results of the readlink call.
Also, if PATH_MAX is not defined it uses a default value of 1024
as for other stdlib implementations.
The expected stack usage is about 8k on Linux where PATH_MAX is
define as 4096 (plus some internal function usage for local
variable).
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
stdlib/Makefile | 3 +-
stdlib/canonicalize.c | 29 +++--
stdlib/tst-canon-bz26341.c | 108 ++++++++++++++++++
support/support_set_small_thread_stack_size.c | 12 +-
support/xthread.h | 2 +
5 files changed, 134 insertions(+), 20 deletions(-)
create mode 100644 stdlib/tst-canon-bz26341.c
Comments
Ping.
On 10/09/2020 12:19, Adhemerval Zanella wrote:
> Changes from previous version [1]:
>
> - I have abandoned the scratch_buffer usage, the PATH_MAX stack usage
> is acceptable and it avoid the potentially malloc usage.
>
> [1] https://sourceware.org/pipermail/libc-alpha/2020-August/117078.html
>
> ---
>
> It uses both a fixed internal buffer with PATH_MAX size to read and
> copy the results of the readlink call.
>
> Also, if PATH_MAX is not defined it uses a default value of 1024
> as for other stdlib implementations.
>
> The expected stack usage is about 8k on Linux where PATH_MAX is
> define as 4096 (plus some internal function usage for local
> variable).
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> stdlib/Makefile | 3 +-
> stdlib/canonicalize.c | 29 +++--
> stdlib/tst-canon-bz26341.c | 108 ++++++++++++++++++
> support/support_set_small_thread_stack_size.c | 12 +-
> support/xthread.h | 2 +
> 5 files changed, 134 insertions(+), 20 deletions(-)
> create mode 100644 stdlib/tst-canon-bz26341.c
>
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 4615f6dfe7..7093b8a584 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-canon-bz26341
>
> tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
> tst-tls-atexit tst-tls-atexit-nodelete
> @@ -102,6 +102,7 @@ LDLIBS-test-atexit-race = $(shared-thread-library)
> LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
> LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
> LDLIBS-test-on_exit-race = $(shared-thread-library)
> +LDLIBS-tst-canon-bz26341 = $(shared-thread-library)
>
> LDLIBS-test-dlclose-exit-race = $(shared-thread-library) $(libdl)
> LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic)
> diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
> index c58439b3fd..6798ed8963 100644
> --- a/stdlib/canonicalize.c
> +++ b/stdlib/canonicalize.c
> @@ -29,6 +29,14 @@
> #include <eloop-threshold.h>
> #include <shlib-compat.h>
>
> +#ifndef PATH_MAX
> +# ifdef MAXPATHLEN
> +# define PATH_MAX MAXPATHLEN
> +# else
> +# define PATH_MAX 1024
> +# endif
> +#endif
> +
> /* Return the canonical absolute name of file NAME. A canonical name
> does not contain any ".", ".." components nor any repeated path
> separators ('/') or symlinks. All path components must exist. If
> @@ -43,10 +51,11 @@
> char *
> __realpath (const char *name, char *resolved)
> {
> - char *rpath, *dest, *extra_buf = NULL;
> + char *rpath, *dest;
> const char *start, *end, *rpath_limit;
> - long int path_max;
> + const size_t path_max = PATH_MAX;
> int num_links = 0;
> + char extra_buf[PATH_MAX];
>
> if (name == NULL)
> {
> @@ -66,14 +75,6 @@ __realpath (const char *name, char *resolved)
> return NULL;
> }
>
> -#ifdef PATH_MAX
> - path_max = PATH_MAX;
> -#else
> - path_max = __pathconf (name, _PC_PATH_MAX);
> - if (path_max <= 0)
> - path_max = 8192;
> -#endif
> -
> if (resolved == NULL)
> {
> rpath = malloc (path_max);
> @@ -170,24 +171,20 @@ __realpath (const char *name, char *resolved)
>
> if (S_ISLNK (st.st_mode))
> {
> - char *buf = __alloca (path_max);
> + char buf[PATH_MAX];
> size_t len;
> ssize_t n;
>
> if (++num_links > __eloop_threshold ())
> {
> __set_errno (ELOOP);
> - goto error;
> - }
> + goto error; }
>
> n = __readlink (rpath, buf, path_max - 1);
> if (n < 0)
> goto error;
> buf[n] = '\0';
>
> - if (!extra_buf)
> - extra_buf = __alloca (path_max);
> -
> len = strlen (end);
> /* Check that n + len + 1 doesn't overflow and is <= path_max. */
> if (n >= SIZE_MAX - len || n + len >= path_max)
> diff --git a/stdlib/tst-canon-bz26341.c b/stdlib/tst-canon-bz26341.c
> new file mode 100644
> index 0000000000..63474bddaa
> --- /dev/null
> +++ b/stdlib/tst-canon-bz26341.c
> @@ -0,0 +1,108 @@
> +/* Check if realpath does not consume extra stack space based on symlink
> + existance in the path (BZ #26341)
> + 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/>. */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/param.h>
> +
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xunistd.h>
> +#include <support/xthread.h>
> +
> +static char *filename;
> +static size_t filenamelen;
> +static char *linkname;
> +
> +static int
> +maxsymlinks (void)
> +{
> +#ifdef MAXSYMLINKS
> + return MAXSYMLINKS;
> +#else
> + long int sysconf_symloop_max = sysconf (_SC_SYMLOOP_MAX);
> + return sysconf_symloop_max <= 0
> + ? _POSIX_SYMLOOP_MAX
> + : sysconf_symloop_max;
> +#endif
> +}
> +
> +#ifndef PATH_MAX
> +# define PATH_MAX 1024
> +#endif
> +
> +static void
> +create_link (void)
> +{
> + int fd = create_temp_file ("tst-canon-bz26341", &filename);
> + TEST_VERIFY_EXIT (fd != -1);
> + xclose (fd);
> +
> + char *prevlink = filename;
> + int maxlinks = maxsymlinks ();
> + for (int i = 0; i < maxlinks; i++)
> + {
> + linkname = xasprintf ("%s%d", filename, i);
> + xsymlink (prevlink, linkname);
> + add_temp_file (linkname);
> + prevlink = linkname;
> + }
> +
> + filenamelen = strlen (filename);
> +}
> +
> +static void *
> +do_realpath (void *arg)
> +{
> + /* Old implementation of realpath allocates a PATH_MAX using alloca
> + for each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
> + maximum stack usage.
> + This stack allocations tries fill the thread allocated stack minus
> + both the thread (plus some slack) and the realpath (plus some slack).
> + If realpath uses more than 2 * PATH_MAX plus some slack it will trigger
> + a stackoverflow. */
> +
> + const size_t realpath_usage = 2 * PATH_MAX + 1024;
> + const size_t thread_usage = 1 * PATH_MAX + 1024;
> + size_t stack_size = support_small_thread_stack_size ()
> + - realpath_usage - thread_usage;
> + char stack[stack_size];
> + char *resolved = stack + stack_size - thread_usage + 1024;
> +
> + char *p = realpath (linkname, resolved);
> + TEST_VERIFY (p != NULL);
> + TEST_COMPARE_BLOB (resolved, filenamelen, filename, filenamelen);
> +
> + return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> + create_link ();
> +
> + pthread_t th = xpthread_create (support_small_stack_thread_attribute (),
> + do_realpath, NULL);
> + xpthread_join (th);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/support/support_set_small_thread_stack_size.c b/support/support_set_small_thread_stack_size.c
> index 69d66e97db..74a0e38a72 100644
> --- a/support/support_set_small_thread_stack_size.c
> +++ b/support/support_set_small_thread_stack_size.c
> @@ -20,8 +20,8 @@
> #include <pthread.h>
> #include <support/xthread.h>
>
> -void
> -support_set_small_thread_stack_size (pthread_attr_t *attr)
> +size_t
> +support_small_thread_stack_size (void)
> {
> /* Some architectures have too small values for PTHREAD_STACK_MIN
> which cannot be used for creating threads. Ensure that the stack
> @@ -31,5 +31,11 @@ support_set_small_thread_stack_size (pthread_attr_t *attr)
> if (stack_size < PTHREAD_STACK_MIN)
> stack_size = PTHREAD_STACK_MIN;
> #endif
> - xpthread_attr_setstacksize (attr, stack_size);
> + return stack_size;
> +}
> +
> +void
> +support_set_small_thread_stack_size (pthread_attr_t *attr)
> +{
> + xpthread_attr_setstacksize (attr, support_small_thread_stack_size ());
> }
> diff --git a/support/xthread.h b/support/xthread.h
> index 05f8d4a7d9..6ba2f5a18b 100644
> --- a/support/xthread.h
> +++ b/support/xthread.h
> @@ -78,6 +78,8 @@ void xpthread_attr_setguardsize (pthread_attr_t *attr,
> /* Set the stack size in ATTR to a small value, but still large enough
> to cover most internal glibc stack usage. */
> void support_set_small_thread_stack_size (pthread_attr_t *attr);
> +/* Return the stack size used on support_set_small_thread_stack_size. */
> +size_t support_small_thread_stack_size (void);
>
> /* Return a pointer to a thread attribute which requests a small
> stack. The caller must not free this pointer. */
>
@@ -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-canon-bz26341
tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
tst-tls-atexit tst-tls-atexit-nodelete
@@ -102,6 +102,7 @@ LDLIBS-test-atexit-race = $(shared-thread-library)
LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
LDLIBS-test-on_exit-race = $(shared-thread-library)
+LDLIBS-tst-canon-bz26341 = $(shared-thread-library)
LDLIBS-test-dlclose-exit-race = $(shared-thread-library) $(libdl)
LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic)
@@ -29,6 +29,14 @@
#include <eloop-threshold.h>
#include <shlib-compat.h>
+#ifndef PATH_MAX
+# ifdef MAXPATHLEN
+# define PATH_MAX MAXPATHLEN
+# else
+# define PATH_MAX 1024
+# endif
+#endif
+
/* Return the canonical absolute name of file NAME. A canonical name
does not contain any ".", ".." components nor any repeated path
separators ('/') or symlinks. All path components must exist. If
@@ -43,10 +51,11 @@
char *
__realpath (const char *name, char *resolved)
{
- char *rpath, *dest, *extra_buf = NULL;
+ char *rpath, *dest;
const char *start, *end, *rpath_limit;
- long int path_max;
+ const size_t path_max = PATH_MAX;
int num_links = 0;
+ char extra_buf[PATH_MAX];
if (name == NULL)
{
@@ -66,14 +75,6 @@ __realpath (const char *name, char *resolved)
return NULL;
}
-#ifdef PATH_MAX
- path_max = PATH_MAX;
-#else
- path_max = __pathconf (name, _PC_PATH_MAX);
- if (path_max <= 0)
- path_max = 8192;
-#endif
-
if (resolved == NULL)
{
rpath = malloc (path_max);
@@ -170,24 +171,20 @@ __realpath (const char *name, char *resolved)
if (S_ISLNK (st.st_mode))
{
- char *buf = __alloca (path_max);
+ char buf[PATH_MAX];
size_t len;
ssize_t n;
if (++num_links > __eloop_threshold ())
{
__set_errno (ELOOP);
- goto error;
- }
+ goto error; }
n = __readlink (rpath, buf, path_max - 1);
if (n < 0)
goto error;
buf[n] = '\0';
- if (!extra_buf)
- extra_buf = __alloca (path_max);
-
len = strlen (end);
/* Check that n + len + 1 doesn't overflow and is <= path_max. */
if (n >= SIZE_MAX - len || n + len >= path_max)
new file mode 100644
@@ -0,0 +1,108 @@
+/* Check if realpath does not consume extra stack space based on symlink
+ existance in the path (BZ #26341)
+ 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/>. */
+
+#include <stdlib.h>
+#include <string.h>
+#include <sys/param.h>
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+#include <support/xthread.h>
+
+static char *filename;
+static size_t filenamelen;
+static char *linkname;
+
+static int
+maxsymlinks (void)
+{
+#ifdef MAXSYMLINKS
+ return MAXSYMLINKS;
+#else
+ long int sysconf_symloop_max = sysconf (_SC_SYMLOOP_MAX);
+ return sysconf_symloop_max <= 0
+ ? _POSIX_SYMLOOP_MAX
+ : sysconf_symloop_max;
+#endif
+}
+
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
+static void
+create_link (void)
+{
+ int fd = create_temp_file ("tst-canon-bz26341", &filename);
+ TEST_VERIFY_EXIT (fd != -1);
+ xclose (fd);
+
+ char *prevlink = filename;
+ int maxlinks = maxsymlinks ();
+ for (int i = 0; i < maxlinks; i++)
+ {
+ linkname = xasprintf ("%s%d", filename, i);
+ xsymlink (prevlink, linkname);
+ add_temp_file (linkname);
+ prevlink = linkname;
+ }
+
+ filenamelen = strlen (filename);
+}
+
+static void *
+do_realpath (void *arg)
+{
+ /* Old implementation of realpath allocates a PATH_MAX using alloca
+ for each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
+ maximum stack usage.
+ This stack allocations tries fill the thread allocated stack minus
+ both the thread (plus some slack) and the realpath (plus some slack).
+ If realpath uses more than 2 * PATH_MAX plus some slack it will trigger
+ a stackoverflow. */
+
+ const size_t realpath_usage = 2 * PATH_MAX + 1024;
+ const size_t thread_usage = 1 * PATH_MAX + 1024;
+ size_t stack_size = support_small_thread_stack_size ()
+ - realpath_usage - thread_usage;
+ char stack[stack_size];
+ char *resolved = stack + stack_size - thread_usage + 1024;
+
+ char *p = realpath (linkname, resolved);
+ TEST_VERIFY (p != NULL);
+ TEST_COMPARE_BLOB (resolved, filenamelen, filename, filenamelen);
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ create_link ();
+
+ pthread_t th = xpthread_create (support_small_stack_thread_attribute (),
+ do_realpath, NULL);
+ xpthread_join (th);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
@@ -20,8 +20,8 @@
#include <pthread.h>
#include <support/xthread.h>
-void
-support_set_small_thread_stack_size (pthread_attr_t *attr)
+size_t
+support_small_thread_stack_size (void)
{
/* Some architectures have too small values for PTHREAD_STACK_MIN
which cannot be used for creating threads. Ensure that the stack
@@ -31,5 +31,11 @@ support_set_small_thread_stack_size (pthread_attr_t *attr)
if (stack_size < PTHREAD_STACK_MIN)
stack_size = PTHREAD_STACK_MIN;
#endif
- xpthread_attr_setstacksize (attr, stack_size);
+ return stack_size;
+}
+
+void
+support_set_small_thread_stack_size (pthread_attr_t *attr)
+{
+ xpthread_attr_setstacksize (attr, support_small_thread_stack_size ());
}
@@ -78,6 +78,8 @@ void xpthread_attr_setguardsize (pthread_attr_t *attr,
/* Set the stack size in ATTR to a small value, but still large enough
to cover most internal glibc stack usage. */
void support_set_small_thread_stack_size (pthread_attr_t *attr);
+/* Return the stack size used on support_set_small_thread_stack_size. */
+size_t support_small_thread_stack_size (void);
/* Return a pointer to a thread attribute which requests a small
stack. The caller must not free this pointer. */