From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from donkey.elm.relay.mailchannels.net (donkey.elm.relay.mailchannels.net [23.83.212.49]) by sourceware.org (Postfix) with ESMTPS id 2D4D838515CD for ; Thu, 30 Jun 2022 03:51:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2D4D838515CD Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id DE51A7612B8; Thu, 30 Jun 2022 03:51:38 +0000 (UTC) Received: from pdx1-sub0-mail-a306.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 6B67876131D; Thu, 30 Jun 2022 03:51:38 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1656561098; a=rsa-sha256; cv=none; b=cjELP9KH7oDONnQH/TYjqz5AIxfU0BrMsm7/3ueSibKbvcM3YKvqDHBG0HKQ9eTiZojriO o/GlEBKZKBCODOvzBCszcm76BD6T8nR3Q2Eu2HATSlexELid+dUSs+fAkNw/1oOaEtyimv kq+O4I3aM864ZF4G7Ly72VPXOq0DX6rLQLNdWB0spj2itQU7wxds3+VEsjD7KUki5dUg43 cHbRktarG3MD3tLUNmIpfpZDNzVvx0fNgcIXOmI1b+47uQ2xykItEqgsz8fMETVHdpO5ts vdCMNPHyv1r0a/vqpb+Q1F6nRWTKKTZ0uVkJdYlF+O2rRfqKbgbZCxx8tEvFLA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1656561098; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=OSiFfcE/T03o6IxZkMiDYJHIYeHWjAGWm9UtBn6EzwU=; b=TAwvQy7R1uLGSmYthVHLFUnL+ni/sdCTTvMt9YYyq+C0B4OxSN/GitE/GiZyw99vTjF8PI EgmU1HTXvWO97Kl1lxSW49rZqyDGW8IcXIm0GD61Gl7eAZBeGoE0bS2r6dDhRFt2bwvISE 3pP2H9rWi0tDTOL9d8dtL8qdGRBur45IGVyGdDhs0xEuUuH2/lA6WNoqgi9d1HPDBNg8wU Q5kvDyYdm3hjbko6cUTYqSG72Ir+ErbrkK1wEMzhVJ5mdEygKif27EjVoalh/Kso9v/mM+ bXQCYbsmKXT+MSCjvU1rfyhWiLs8EDMrsLJt/beW1m4k5j1vI2f4SzZIQTO+1g== ARC-Authentication-Results: i=1; rspamd-689699966c-dn4bp; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Dime-Continue: 6d6c81bf5307c42e_1656561098691_2293725135 X-MC-Loop-Signature: 1656561098691:77178415 X-MC-Ingress-Time: 1656561098691 Received: from pdx1-sub0-mail-a306.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.125.123.46 (trex/6.7.1); Thu, 30 Jun 2022 03:51:38 +0000 Received: from [192.168.1.174] (unknown [1.186.122.42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a306.dreamhost.com (Postfix) with ESMTPSA id 4LYPX80Qk0zj4; Wed, 29 Jun 2022 20:51:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1656561098; bh=OSiFfcE/T03o6IxZkMiDYJHIYeHWjAGWm9UtBn6EzwU=; h=Date:Subject:To:From:Content-Type:Content-Transfer-Encoding; b=WcSYpy1ORI6dSJBgdg1XCOZio/yK5TXLRLft3E/tRClj27eRQz2OHvcPppJQVw3vV QsGGYCVEDaXPXBrOmuQexyaQkz6l/C+7D6N5ibP6C2ekyAdY/w/BUqWqYhd4MpcHPJ UFG0ekIzw4HIhW5LQHsHZS2PRyNo4kvVM8SMzFP08GHnB9qpqWcHeXtVEPtVCpkwdn Ena/bZKtCM0QPKY2u79DSy3aWtM+M06mGMz6JWeIkyVPcuGGQrUi7zqRgPmPuHnRAR cEjx0mkYi+bFYTOJIek6eXGyFoWy+zO+zlFf3iDrT6GFh72rYfpmLY1IemDU4Kkrn/ t9OTZyIlniERw== Message-ID: <301547b9-251d-738f-2791-970bbe2ff511@gotplt.org> Date: Thu, 30 Jun 2022 09:21:30 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH] stdlib: Simplify buffer management in canonicalize Content-Language: en-US To: Florian Weimer , libc-alpha@sourceware.org References: <87wnd1f1ul.fsf@oldenburg.str.redhat.com> From: Siddhesh Poyarekar In-Reply-To: <87wnd1f1ul.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3037.3 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_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, RCVD_IN_SBL, 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: Thu, 30 Jun 2022 03:51:42 -0000 On 28/06/2022 17:15, Florian Weimer via Libc-alpha wrote: > Move the buffer management from realpath_stk to __realpath. This > allows returning directly after allocation errors. > > Always make a copy of the result buffer using strdup even if it is > already heap-allocated. (Heap-allocated buffers are somewhat rare.) > This avoids GCC warnings at certain optimization levels. LGTM. Reviewed-by: Siddhesh Poyarekar > > Tested on i686-linux-gnu and x86_64-linux-gnu. Built with > build-many-glibcs.py. > > --- > stdlib/canonicalize.c | 113 ++++++++++++++++++++++---------------------------- > 1 file changed, 50 insertions(+), 63 deletions(-) > > diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c > index e6566bd7d9..54dfac454c 100644 > --- a/stdlib/canonicalize.c > +++ b/stdlib/canonicalize.c > @@ -49,6 +49,7 @@ > #else > # define __canonicalize_file_name canonicalize_file_name > # define __realpath realpath > +# define __strdup strdup > # include "pathmax.h" > # define __faccessat faccessat > # if defined _WIN32 && !defined __CYGWIN__ > @@ -176,27 +177,16 @@ get_path_max (void) > return path_max < 0 ? 1024 : path_max <= IDX_MAX ? path_max : IDX_MAX; > } > > -/* Act like __realpath (see below), with an additional argument > - rname_buf that can be used as temporary storage. > +/* Scratch buffers used by realpath_stk and managed by __realpath. */ > +struct realpath_bufs > +{ > + struct scratch_buffer rname; > + struct scratch_buffer extra; > + struct scratch_buffer link; > +}; > > - If GCC_LINT is defined, do not inline this function with GCC 10.1 > - and later, to avoid creating a pointer to the stack that GCC > - -Wreturn-local-addr incorrectly complains about. See: > - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644 > - Although the noinline attribute can hurt performance a bit, no better way > - to pacify GCC is known; even an explicit #pragma does not pacify GCC. > - When the GCC bug is fixed this workaround should be limited to the > - broken GCC versions. */ > -#if __GNUC_PREREQ (10, 1) > -# if defined GCC_LINT || defined lint > -__attribute__ ((__noinline__)) > -# elif __OPTIMIZE__ && !__NO_INLINE__ > -# define GCC_BOGUS_WRETURN_LOCAL_ADDR > -# endif > -#endif > static char * > -realpath_stk (const char *name, char *resolved, > - struct scratch_buffer *rname_buf) > +realpath_stk (const char *name, char *resolved, struct realpath_bufs *bufs) > { > char *dest; > char const *start; > @@ -221,12 +211,7 @@ realpath_stk (const char *name, char *resolved, > return NULL; > } > > - struct scratch_buffer extra_buffer, link_buffer; > - scratch_buffer_init (&extra_buffer); > - scratch_buffer_init (&link_buffer); > - scratch_buffer_init (rname_buf); > - char *rname_on_stack = rname_buf->data; > - char *rname = rname_on_stack; > + char *rname = bufs->rname.data; > bool end_in_extra_buffer = false; > bool failed = true; > > @@ -236,16 +221,16 @@ realpath_stk (const char *name, char *resolved, > > if (!IS_ABSOLUTE_FILE_NAME (name)) > { > - while (!__getcwd (rname, rname_buf->length)) > + while (!__getcwd (bufs->rname.data, bufs->rname.length)) > { > if (errno != ERANGE) > { > dest = rname; > goto error; > } > - if (!scratch_buffer_grow (rname_buf)) > - goto error_nomem; > - rname = rname_buf->data; > + if (!scratch_buffer_grow (&bufs->rname)) > + return NULL; > + rname = bufs->rname.data; > } > dest = __rawmemchr (rname, '\0'); > start = name; > @@ -299,13 +284,13 @@ realpath_stk (const char *name, char *resolved, > if (!ISSLASH (dest[-1])) > *dest++ = '/'; > > - while (rname + rname_buf->length - dest > + while (rname + bufs->rname.length - dest > < startlen + sizeof dir_suffix) > { > idx_t dest_offset = dest - rname; > - if (!scratch_buffer_grow_preserve (rname_buf)) > - goto error_nomem; > - rname = rname_buf->data; > + if (!scratch_buffer_grow_preserve (&bufs->rname)) > + return NULL; > + rname = bufs->rname.data; > dest = rname + dest_offset; > } > > @@ -316,13 +301,13 @@ realpath_stk (const char *name, char *resolved, > ssize_t n; > while (true) > { > - buf = link_buffer.data; > - idx_t bufsize = link_buffer.length; > + buf = bufs->link.data; > + idx_t bufsize = bufs->link.length; > n = __readlink (rname, buf, bufsize - 1); > if (n < bufsize - 1) > break; > - if (!scratch_buffer_grow (&link_buffer)) > - goto error_nomem; > + if (!scratch_buffer_grow (&bufs->link)) > + return NULL; > } > if (0 <= n) > { > @@ -334,7 +319,7 @@ realpath_stk (const char *name, char *resolved, > > buf[n] = '\0'; > > - char *extra_buf = extra_buffer.data; > + char *extra_buf = bufs->extra.data; > idx_t end_idx IF_LINT (= 0); > if (end_in_extra_buffer) > end_idx = end - extra_buf; > @@ -342,13 +327,13 @@ realpath_stk (const char *name, char *resolved, > if (INT_ADD_OVERFLOW (len, n)) > { > __set_errno (ENOMEM); > - goto error_nomem; > + return NULL; > } > - while (extra_buffer.length <= len + n) > + while (bufs->extra.length <= len + n) > { > - if (!scratch_buffer_grow_preserve (&extra_buffer)) > - goto error_nomem; > - extra_buf = extra_buffer.data; > + if (!scratch_buffer_grow_preserve (&bufs->extra)) > + return NULL; > + extra_buf = bufs->extra.data; > } > if (end_in_extra_buffer) > end = extra_buf + end_idx; > @@ -406,25 +391,24 @@ error: > to the path not existing or not being accessible. */ > if ((!failed || errno == ENOENT || errno == EACCES) > && dest - rname <= get_path_max ()) > - rname = strcpy (resolved, rname); > - else if (!failed) > { > - failed = true; > - __set_errno (ENAMETOOLONG); > + strcpy (resolved, rname); > + if (failed) > + return NULL; > + else > + return resolved; > } > + if (!failed) > + __set_errno (ENAMETOOLONG); > + return NULL; > } > - > -error_nomem: > - scratch_buffer_free (&extra_buffer); > - scratch_buffer_free (&link_buffer); > - > - if (failed || rname == resolved) > + else > { > - scratch_buffer_free (rname_buf); > - return failed ? NULL : resolved; > + if (failed) > + return NULL; > + else > + return __strdup (bufs->rname.data); > } > - > - return scratch_buffer_dupfree (rname_buf, dest - rname); > } > > /* Return the canonical absolute name of file NAME. A canonical name > @@ -441,12 +425,15 @@ error_nomem: > char * > __realpath (const char *name, char *resolved) > { > - #ifdef GCC_BOGUS_WRETURN_LOCAL_ADDR > - #warning "GCC might issue a bogus -Wreturn-local-addr warning here." > - #warning "See ." > - #endif > - struct scratch_buffer rname_buffer; > - return realpath_stk (name, resolved, &rname_buffer); > + struct realpath_bufs bufs; > + scratch_buffer_init (&bufs.rname); > + scratch_buffer_init (&bufs.extra); > + scratch_buffer_init (&bufs.link); > + char *result = realpath_stk (name, resolved, &bufs); > + scratch_buffer_free (&bufs.link); > + scratch_buffer_free (&bufs.extra); > + scratch_buffer_free (&bufs.rname); > + return result; > } > libc_hidden_def (__realpath) > versioned_symbol (libc, __realpath, realpath, GLIBC_2_3); >