From patchwork Tue Jun 28 11:45:38 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 55492 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 22C69396E863 for ; Tue, 28 Jun 2022 14:38:06 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 22C69396E863 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1656427086; bh=SBbOqIBp7l9R/axN6WS1vrEHr7m3sRVG1b1w7mWnv54=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=jCg4lvRuMpIo1A7dW7VCkG/p/RR+c5iNKPCycgUZj9W6aIwsOY27jvcm52zS09n5y weQEODSpckmUJ5aaHvOP3xq200gXRjW3LZbyJowIqSxEhHTMjUxj7RDGHF/Gqq3E8Y UaOlHuL+oKeqUnfr12EPEzt7ppvE4+KQzswc0D0M= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id A7AAA3853559 for ; Tue, 28 Jun 2022 11:45:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A7AAA3853559 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-471-EF6gTtdiPUmvIV1_MtHZrQ-1; Tue, 28 Jun 2022 07:45:42 -0400 X-MC-Unique: EF6gTtdiPUmvIV1_MtHZrQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B24331019C8A for ; Tue, 28 Jun 2022 11:45:41 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.39.193.0]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DEC0140466B0 for ; Tue, 28 Jun 2022 11:45:40 +0000 (UTC) To: libc-alpha@sourceware.org Subject: [PATCH] stdlib: Simplify buffer management in canonicalize Date: Tue, 28 Jun 2022 13:45:38 +0200 Message-ID: <87wnd1f1ul.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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-Patchwork-Original-From: Florian Weimer via Libc-alpha From: Florian Weimer Reply-To: Florian Weimer Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" 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. Tested on i686-linux-gnu and x86_64-linux-gnu. Built with build-many-glibcs.py. Reviewed-by: Siddhesh Poyarekar --- 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);