From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x843.google.com (mail-qt1-x843.google.com [IPv6:2607:f8b0:4864:20::843]) by sourceware.org (Postfix) with ESMTPS id 6ED1C3836C73 for ; Wed, 25 Nov 2020 15:21:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6ED1C3836C73 Received: by mail-qt1-x843.google.com with SMTP id z3so1812494qtw.9 for ; Wed, 25 Nov 2020 07:21:34 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:cc:references:from:autocrypt:subject :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=Ftm8choIJ7OD4TbNIuW+poWz5OLcj0dZ6Ms9/RQJorM=; b=Go2QKsSEO7SkwV2Bj6lcgerIjwGoKR2j870MUzXoUPSryBwZHyFQBRYDpmZoijqBot I85mcXGb1TLviZESraCJ1Wn1Wnb0jC2oXWjon+VovqJOpcYaMiS8h0ktTExEYdk6aXUK hZsUdGFX60S1/0lyU0KnkO/WCfOtqo0Wx+f1Pg3V8EZ2cjZoIrU8LPIlzcWOigeRMPE/ lJ9FgxuAoqzvcAnAY2Kf6aDlacRxar/W79XsgK6guJKbTbikoBwVhVHkDFiKNzeIIjWb WTY7mNZAYwfHPh0QUqEEMQgSG/kTT9IXNNoC/dmm1Xfs0We/ICcAisQzA0ZjwRweWDta 6J8A== X-Gm-Message-State: AOAM531QgzaUH2pMHN4DdvznEG9Kxa86RrTjix2+CFAKKgZNlclzTpI1 Pv+Q2l5NEwzGbb+gSySj1KiHwHX1NWpSFg== X-Google-Smtp-Source: ABdhPJwQNfNs2X3F/VGhY0Tk+y7SvEzlOCdjriWPAAiDOhv9UKWmDTm+5k6tIHtiEtTXh2pB9y1R0Q== X-Received: by 2002:ac8:7b4c:: with SMTP id m12mr1971113qtu.242.1606317693981; Wed, 25 Nov 2020 07:21:33 -0800 (PST) Received: from [192.168.1.4] ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id j202sm2452101qke.108.2020.11.25.07.21.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Nov 2020 07:21:33 -0800 (PST) To: Xiaoming Ni , libc-alpha@sourceware.org, carlos@redhat.com, glibc-bugs@sourceware.org Cc: wangle6@huawei.com References: <20201125100659.30923-1-nixiaoming@huawei.com> From: Adhemerval Zanella Autocrypt: addr=adhemerval.zanella@linaro.org; prefer-encrypt=mutual; keydata= mQINBFcVGkoBEADiQU2x/cBBmAVf5C2d1xgz6zCnlCefbqaflUBw4hB/bEME40QsrVzWZ5Nq 8kxkEczZzAOKkkvv4pRVLlLn/zDtFXhlcvQRJ3yFMGqzBjofucOrmdYkOGo0uCaoJKPT186L NWp53SACXguFJpnw4ODI64ziInzXQs/rUJqrFoVIlrPDmNv/LUv1OVPKz20ETjgfpg8MNwG6 iMizMefCl+RbtXbIEZ3TE/IaDT/jcOirjv96lBKrc/pAL0h/O71Kwbbp43fimW80GhjiaN2y WGByepnkAVP7FyNarhdDpJhoDmUk9yfwNuIuESaCQtfd3vgKKuo6grcKZ8bHy7IXX1XJj2X/ BgRVhVgMHAnDPFIkXtP+SiarkUaLjGzCz7XkUn4XAGDskBNfbizFqYUQCaL2FdbW3DeZqNIa nSzKAZK7Dm9+0VVSRZXP89w71Y7JUV56xL/PlOE+YKKFdEw+gQjQi0e+DZILAtFjJLoCrkEX w4LluMhYX/X8XP6/C3xW0yOZhvHYyn72sV4yJ1uyc/qz3OY32CRy+bwPzAMAkhdwcORA3JPb kPTlimhQqVgvca8m+MQ/JFZ6D+K7QPyvEv7bQ7M+IzFmTkOCwCJ3xqOD6GjX3aphk8Sr0dq3 4Awlf5xFDAG8dn8Uuutb7naGBd/fEv6t8dfkNyzj6yvc4jpVxwARAQABtElBZGhlbWVydmFs IFphbmVsbGEgTmV0dG8gKExpbmFybyBWUE4gS2V5KSA8YWRoZW1lcnZhbC56YW5lbGxhQGxp bmFyby5vcmc+iQI3BBMBCAAhBQJXFRpKAhsDBQsJCAcDBRUKCQgLBRYCAwEAAh4BAheAAAoJ EKqx7BSnlIjv0e8P/1YOYoNkvJ+AJcNUaM5a2SA9oAKjSJ/M/EN4Id5Ow41ZJS4lUA0apSXW NjQg3VeVc2RiHab2LIB4MxdJhaWTuzfLkYnBeoy4u6njYcaoSwf3g9dSsvsl3mhtuzm6aXFH /Qsauav77enJh99tI4T+58rp0EuLhDsQbnBic/ukYNv7sQV8dy9KxA54yLnYUFqH6pfH8Lly sTVAMyi5Fg5O5/hVV+Z0Kpr+ZocC1YFJkTsNLAW5EIYSP9ftniqaVsim7MNmodv/zqK0IyDB GLLH1kjhvb5+6ySGlWbMTomt/or/uvMgulz0bRS+LUyOmlfXDdT+t38VPKBBVwFMarNuREU2 69M3a3jdTfScboDd2ck1u7l+QbaGoHZQ8ZNUrzgObltjohiIsazqkgYDQzXIMrD9H19E+8fw kCNUlXxjEgH/Kg8DlpoYJXSJCX0fjMWfXywL6ZXc2xyG/hbl5hvsLNmqDpLpc1CfKcA0BkK+ k8R57fr91mTCppSwwKJYO9T+8J+o4ho/CJnK/jBy1pWKMYJPvvrpdBCWq3MfzVpXYdahRKHI ypk8m4QlRlbOXWJ3TDd/SKNfSSrWgwRSg7XCjSlR7PNzNFXTULLB34sZhjrN6Q8NQZsZnMNs TX8nlGOVrKolnQPjKCLwCyu8PhllU8OwbSMKskcD1PSkG6h3r0AquQINBFcVGkoBEACgAdbR Ck+fsfOVwT8zowMiL3l9a2DP3Eeak23ifdZG+8Avb/SImpv0UMSbRfnw/N81IWwlbjkjbGTu oT37iZHLRwYUFmA8fZX0wNDNKQUUTjN6XalJmvhdz9l71H3WnE0wneEM5ahu5V1L1utUWTyh VUwzX1lwJeV3vyrNgI1kYOaeuNVvq7npNR6t6XxEpqPsNc6O77I12XELic2+36YibyqlTJIQ V1SZEbIy26AbC2zH9WqaKyGyQnr/IPbTJ2Lv0dM3RaXoVf+CeK7gB2B+w1hZummD21c1Laua +VIMPCUQ+EM8W9EtX+0iJXxI+wsztLT6vltQcm+5Q7tY+HFUucizJkAOAz98YFucwKefbkTp eKvCfCwiM1bGatZEFFKIlvJ2QNMQNiUrqJBlW9nZp/k7pbG3oStOjvawD9ZbP9e0fnlWJIsj 6c7pX354Yi7kxIk/6gREidHLLqEb/otuwt1aoMPg97iUgDV5mlNef77lWE8vxmlY0FBWIXuZ yv0XYxf1WF6dRizwFFbxvUZzIJp3spAao7jLsQj1DbD2s5+S1BW09A0mI/1DjB6EhNN+4bDB SJCOv/ReK3tFJXuj/HbyDrOdoMt8aIFbe7YFLEExHpSk+HgN05Lg5TyTro8oW7TSMTk+8a5M kzaH4UGXTTBDP/g5cfL3RFPl79ubXwARAQABiQIfBBgBCAAJBQJXFRpKAhsMAAoJEKqx7BSn lIjvI/8P/jg0jl4Tbvg3B5kT6PxJOXHYu9OoyaHLcay6Cd+ZrOd1VQQCbOcgLFbf4Yr+rE9l mYsY67AUgq2QKmVVbn9pjvGsEaz8UmfDnz5epUhDxC6yRRvY4hreMXZhPZ1pbMa6A0a/WOSt AgFj5V6Z4dXGTM/lNManr0HjXxbUYv2WfbNt3/07Db9T+GZkpUotC6iknsTA4rJi6u2ls0W9 1UIvW4o01vb4nZRCj4rni0g6eWoQCGoVDk/xFfy7ZliR5B+3Z3EWRJcQskip/QAHjbLa3pml xAZ484fVxgeESOoaeC9TiBIp0NfH8akWOI0HpBCiBD5xaCTvR7ujUWMvhsX2n881r/hNlR9g fcE6q00qHSPAEgGr1bnFv74/1vbKtjeXLCcRKk3Ulw0bY1OoDxWQr86T2fZGJ/HIZuVVBf3+ gaYJF92GXFynHnea14nFFuFgOni0Mi1zDxYH/8yGGBXvo14KWd8JOW0NJPaCDFJkdS5hu0VY 7vJwKcyHJGxsCLU+Et0mryX8qZwqibJIzu7kUJQdQDljbRPDFd/xmGUFCQiQAncSilYOcxNU EMVCXPAQTteqkvA+gNqSaK1NM9tY0eQ4iJpo+aoX8HAcn4sZzt2pfUB9vQMTBJ2d4+m/qO6+ cFTAceXmIoFsN8+gFN3i8Is3u12u8xGudcBPvpoy4OoG Subject: Re: [PATCH v4] io:nftw/ftw:fix stack overflow when large nopenfd [BZ #26353] Message-ID: <895ceb58-1110-b12c-32ce-b9c13e5c8543@linaro.org> Date: Wed, 25 Nov 2020 12:21:30 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201125100659.30923-1-nixiaoming@huawei.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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: glibc-bugs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Glibc-bugs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Nov 2020 15:21:36 -0000 On 25/11/2020 07:06, Xiaoming Ni wrote: > In ftw_startup(), call alloca to apply for a large amount of stack space. > When descriptors is very large, stack overflow is triggered. BZ #26353 > > Replace the memory application of data.dirstreams from alloca() to malloc() > by referring to the memory application of data.dirbuf in the current function. > Combine the memory allocation of data.dirbuf and data.dirstreams according > to the advice of Adhemerval Zanella. > > v4: > Based on Adhemerval Zanella's review comments, > use support_capture_subprocess_check() to rewrite testcase > v3: https://public-inbox.org/libc-alpha/20201124131652.111854-1-nixiaoming@huawei.com/ > Combine the memory allocation of data.dirbuf and data.dirstreams > according to the advice of Adhemerval Zanella. > remove the upper limit check of descriptors > v2: https://public-inbox.org/libc-alpha/20200815070851.46403-1-nixiaoming@huawei.com/ > not set errno after malloc fails. > add check ftw return value on testcase > add more testcase > v1: https://public-inbox.org/libc-alpha/20200808084640.49174-1-nixiaoming@huawei.com/ > ' Looks good, with a small adjustment below. If you are ok with my suggestion I can push on master on your behalf. Thanks. > --- > io/Makefile | 3 +- > io/ftw.c | 16 ++++++----- > io/tst-bz26353.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+), 8 deletions(-) > create mode 100644 io/tst-bz26353.c > > diff --git a/io/Makefile b/io/Makefile > index 6dd2c33fcf..7845d07f45 100644 > --- a/io/Makefile > +++ b/io/Makefile > @@ -68,7 +68,8 @@ tests := test-utime test-stat test-stat2 test-lfs tst-getcwd \ > tst-posix_fallocate tst-posix_fallocate64 \ > tst-fts tst-fts-lfs tst-open-tmpfile \ > tst-copy_file_range tst-getcwd-abspath tst-lockf \ > - tst-ftw-lnk tst-file_change_detection tst-lchmod > + tst-ftw-lnk tst-file_change_detection tst-lchmod \ > + tst-bz26353 > > # Likewise for statx, but we do not need static linking here. > tests-internal += tst-statx Ok. > diff --git a/io/ftw.c b/io/ftw.c > index 7104816e85..92e08c5431 100644 > --- a/io/ftw.c > +++ b/io/ftw.c > @@ -646,15 +646,17 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, > > data.maxdir = descriptors < 1 ? 1 : descriptors; > data.actdir = 0; > - data.dirstreams = (struct dir_data **) alloca (data.maxdir > - * sizeof (struct dir_data *)); > - memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *)); > - > /* PATH_MAX is always defined when we get here. */ > data.dirbufsize = MAX (2 * strlen (dir), PATH_MAX); > - data.dirbuf = (char *) malloc (data.dirbufsize); > - if (data.dirbuf == NULL) > + data.dirstreams = malloc (data.maxdir * sizeof (struct dir_data *) > + + data.dirbufsize); > + if (data.dirstreams == NULL) > return -1; > + > + memset (data.dirstreams, '\0', data.maxdir * sizeof (struct dir_data *)); > + > + data.dirbuf = (char *) data.dirstreams > + + data.maxdir * sizeof (struct dir_data *); > cp = __stpcpy (data.dirbuf, dir); > /* Strip trailing slashes. */ > while (cp > data.dirbuf + 1 && cp[-1] == '/') Ok. > @@ -803,7 +805,7 @@ ftw_startup (const char *dir, int is_nftw, void *func, int descriptors, > out_fail: > save_err = errno; > __tdestroy (data.known_objects, free); > - free (data.dirbuf); > + free (data.dirstreams); > __set_errno (save_err); > > return result; Ok. > diff --git a/io/tst-bz26353.c b/io/tst-bz26353.c > new file mode 100644 > index 0000000000..27f50a8bdc > --- /dev/null > +++ b/io/tst-bz26353.c > @@ -0,0 +1,75 @@ > +/* test ftw bz26353: Check whether stack overflow occurs when the value > + of the nopenfd parameter is too large. > + > + 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 > + . */ > + > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +static int > +my_func (const char *file, const struct stat *sb, int flag) > +{ > + return 0; > +} > + > +static void > +do_ftw (void *unused) > +{ > + char *tempdir = support_create_temp_directory ("tst-bz26353"); > + struct rlimit r; > + int large_nopenfd = 80 * 1024 * 1024; This is will always overwritten by the next assignment, getrlimit will always succeed, unless it is being filtered by seccomp or similar facility. In any case, other glibc will also fail, so we can assume it always available. > + int res = getrlimit (RLIMIT_STACK, &r); > + if (res == 0) Just check the result with TEST_COMPARE. > + { > + if (r.rlim_cur == RLIM_INFINITY) > + { > + r.rlim_cur = 8 * 1024 * 1024; > + setrlimit (RLIMIT_STACK, &r); > + } > + large_nopenfd = (int) r.rlim_cur; Ok, it limiting the stack to 8 MB in any case, otherwise it is setting a fd value that will likely trigger a stack overflow. > + } > + int ret = ftw (tempdir, my_func, large_nopenfd); > + if (ret != 0) > + { > + printf ("test big num %d, ret=%d errno=%d, without stack overflow\n", > + large_nopenfd, ret, errno); > + } Just check with TEST_COMPARE where. I a short I think it can be simplified with: struct rlimit r; TEST_COMPARE (getrlimit (RLIMIT_STACK, &r), 0); if (r.rlim_cur == RLIM_INFINITY) { r.rlim_cur = 8 * 1024 * 1024; TEST_COMPARE (setrlimit (RLIMIT_STACK, &r), 0); } int large_nopenfd = (int) r.rlim_cur; > + free (tempdir); > + exit (ret); > +} > + > +/*Check whether stack overflow occurs*/ > +static int > +do_test (void) > +{ > + struct support_capture_subprocess result; > + result = support_capture_subprocess (do_ftw, NULL); > + support_capture_subprocess_check (&result, "bz26353", 0, sc_allow_none); > + support_capture_subprocess_free (&result); > + return 0; > +} > + > +#include > + Spurious extra line.