From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from huawei.com (szxga05-in.huawei.com [45.249.212.191]) by sourceware.org (Postfix) with ESMTPS id 6EF003851C29; Sat, 8 Aug 2020 09:14:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6EF003851C29 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=nixiaoming@huawei.com Received: from DGGEMS405-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id 74209185AAF6CEF3848A; Sat, 8 Aug 2020 17:14:43 +0800 (CST) Received: from [127.0.0.1] (10.67.102.197) by DGGEMS405-HUB.china.huawei.com (10.3.19.205) with Microsoft SMTP Server id 14.3.487.0; Sat, 8 Aug 2020 17:14:35 +0800 Subject: Re: [PATCH v2] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341] To: Adhemerval Zanella , , , , , , CC: , , Paul Eggert References: <20200807101601.61670-1-nixiaoming@huawei.com> From: Xiaoming Ni Message-ID: Date: Sat, 8 Aug 2020 17:14:34 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.0.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.102.197] X-CFilter-Loop: Reflected X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_MANYTO, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 08 Aug 2020 09:14:48 -0000 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 > > Again, we do not use DCO, but rather Copyright assignment. Do I just need to delete this line: "Signed-off-by: Xiaoming Ni "? 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 . */ > Use it need to use the libsupport (check support/README-testing.c and other > tests that use '#include '. 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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