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

[Xiaoming Ni <nixiaoming@huawei.com>]

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