From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com [IPv6:2607:f8b0:4864:20::b34]) by sourceware.org (Postfix) with ESMTPS id 846B7386C592 for ; Wed, 22 Jun 2022 15:24:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 846B7386C592 Received: by mail-yb1-xb34.google.com with SMTP id i7so13099587ybe.11 for ; Wed, 22 Jun 2022 08:24:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qBYel9gwQViKU7h9SRt7rlv1IYs0RA5/f8tSq9aDUuE=; b=Rb7jrkpl1Dd3b7Fzp7QLRJaW5tv3y/P5rM14wxuFfXfCmWZhjOB6TCeXw0Rc6zfSxt aMG0iRcLgS5iDzq6HNdU/ltj+80v+bQTqjGW3Ud4mkKk+08zdWHHPHkTooFOOJZJW0mN S/OjmwyCPMK7ThicLx7XTQi7woysEWJUvYq/8f9EB/RyOyk/WBLK7NFHHSpKrFA/zn8G WZWmUReBF0T9mvX5sGMwNusA4/6QOaahVk53CI9TBD0qNc+SXKlVrPmgq9NmEAXHEO6y eO9m87S0lvNT9eh0rWQAB9QOUZk8g2yPKPBgNMz/OjpdpRCHPfdBUwmJKmkA7xTQfOa/ Euow== X-Gm-Message-State: AJIora9+EPS9LfVqhhIUZdz0V9pUn+PQrdahCX5M0VuZg+i1m8YF4R/d dzZY+Aw7p7FAbkfwCNOhRnpg6cfxLpkwuiniHwDrtBDZ4k8= X-Google-Smtp-Source: AGRyM1ud4BA1kvdYWPN0rJKhoXnkb92hyI+5+rPN7Mc+UdoRfgRf4pfcCT2uESAuy2kVN43NLrNzz5r2kW0+Ff9ax+g= X-Received: by 2002:a25:b806:0:b0:663:d35d:8b8a with SMTP id v6-20020a25b806000000b00663d35d8b8amr4449411ybj.69.1655911478755; Wed, 22 Jun 2022 08:24:38 -0700 (PDT) MIME-Version: 1.0 References: <20220621161821.2940071-1-goldstein.w.n@gmail.com> <982ee9ea-3169-4eb4-78d1-efc13dc3b672@gotplt.org> In-Reply-To: <982ee9ea-3169-4eb4-78d1-efc13dc3b672@gotplt.org> From: Noah Goldstein Date: Wed, 22 Jun 2022 08:24:27 -0700 Message-ID: Subject: Re: [PATCH v1] stdlib: Remove attr_write from mbstows if dst is NULL [BZ: 29265] To: Siddhesh Poyarekar Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Wed, 22 Jun 2022 15:24:41 -0000 On Wed, Jun 22, 2022 at 12:09 AM Siddhesh Poyarekar wrote: > > On 21/06/2022 21:48, Noah Goldstein via Libc-alpha wrote: > > mbstows is defined if dst is NULL and is defined to special cased if > > dst is NULL so the fortify objsize check if incorrect in that case. > > > > Tested on x86-64 linux. > > --- > > Note. I wasn't able to get the test to actually throw an error > > before the change. > > The test would rely on whether the middle end is able to fold away the > condition late enough, so it's flaky enough for us to not care about it. > It's fine to have it in though as a smoke test. Alright. > > > > > > > stdlib/Makefile | 3 +++ > > stdlib/bits/stdlib.h | 16 +++++++++++----- > > stdlib/testmb.c | 7 +++++++ > > 3 files changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/stdlib/Makefile b/stdlib/Makefile > > index 60fc59c12c..6ef725ef74 100644 > > --- a/stdlib/Makefile > > +++ b/stdlib/Makefile > > @@ -373,6 +373,9 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags) > > CFLAGS-tst-makecontext.c += -funwind-tables > > CFLAGS-tst-makecontext2.c += $(stack-align-test-flags) > > > > +CFLAGS-testmb.c += -D_FORTIFY_SOURCE=2 -Wall -Werror > > + > > + > > # Run a test on the header files we use. > > tests-special += $(objpfx)isomac.out > > > > diff --git a/stdlib/bits/stdlib.h b/stdlib/bits/stdlib.h > > index 277d099e22..9ab66db6a4 100644 > > --- a/stdlib/bits/stdlib.h > > +++ b/stdlib/bits/stdlib.h > > @@ -96,6 +96,11 @@ extern size_t __mbstowcs_chk (wchar_t *__restrict __dst, > > const char *__restrict __src, > > size_t __len, size_t __dstlen) __THROW > > __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2)); > > +extern size_t __REDIRECT_NTH (__mbstowcs_chk_nulldst, > > + (wchar_t *__restrict __dst, > > + const char *__restrict __src, > > + size_t __len), mbstowcs_chk) > > + __attr_access ((__read_only__, 2)); > > extern size_t __REDIRECT_NTH (__mbstowcs_alias, > > (wchar_t *__restrict __dst, > > const char *__restrict __src, > > @@ -108,16 +113,17 @@ extern size_t __REDIRECT_NTH (__mbstowcs_chk_warn, > > __warnattr ("mbstowcs called with dst buffer smaller than len " > > "* sizeof (wchar_t)"); > > > > -__fortify_function size_t > > +__always_inline __fortify_function size_t > > __NTH (mbstowcs (wchar_t *__restrict __dst, const char *__restrict __src, > > size_t __len)) > > { > > - return __glibc_fortify_n (mbstowcs, __len, sizeof (wchar_t), > > - __glibc_objsize (__dst), > > - __dst, __src, __len); > > + if (__builtin_constant_p (__dst) && __dst == NULL) > > Perhaps a better condition would be __builtin_constant_p (__dst == NULL) > && __dst == NULL so that it does not rely on __dst to be a constant. > For example, it could be a variable that the compiler decides can have > only a NULL value. > Bright. Fixed in v2. > > + return __mbstowcs_chk_nulldst (__dst, __src, __len); > > + else > > + return __glibc_fortify_n (mbstowcs, __len, sizeof (wchar_t), > > + __glibc_objsize (__dst), __dst, __src, __len); > > OK. > > > } > > > > - > > extern size_t __wcstombs_chk (char *__restrict __dst, > > const wchar_t *__restrict __src, > > size_t __len, size_t __dstlen) __THROW > > diff --git a/stdlib/testmb.c b/stdlib/testmb.c > > index 45dae7db61..6ac4dfd21d 100644 > > --- a/stdlib/testmb.c > > +++ b/stdlib/testmb.c > > @@ -16,6 +16,13 @@ main (int argc, char *argv[]) > > lose = 1; > > } > > > > + i = mbstowcs (NULL, "bar", 4); > > + if (!(i == 3 && w[1] == 'a')) > > + { > > + puts ("mbstowcs FAILED2!"); > > + lose = 1; > > + } > > + > > mbstowcs (w, "blah", 5); > > i = wcstombs (c, w, 10); > > if (i != 4) > > OK.