Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]

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

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