Re: [PATCH v3 2/4 v2] stdlib: Use fixed buffer size for realpath [BZ #26241]

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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.  */
>