From: Siddhesh Poyarekar <siddhesh@gotplt.org>
To: Florian Weimer <fweimer@redhat.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH] stdlib: Simplify buffer management in canonicalize
Date: Thu, 30 Jun 2022 09:21:30 +0530 [thread overview]
Message-ID: <301547b9-251d-738f-2791-970bbe2ff511@gotplt.org> (raw)
In-Reply-To: <87wnd1f1ul.fsf@oldenburg.str.redhat.com>
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 <siddhesh@sourceware.org>
>
> 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 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644>."
> - #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);
>
prev parent reply other threads:[~2022-06-30 3:51 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-28 11:45 Florian Weimer
2022-06-30 3:51 ` Siddhesh Poyarekar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=301547b9-251d-738f-2791-970bbe2ff511@gotplt.org \
--to=siddhesh@gotplt.org \
--cc=fweimer@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).