diff mbox series

stdlib: Simplify buffer management in canonicalize

Message ID 87wnd1f1ul.fsf@oldenburg.str.redhat.com
State Committed
Headers show
Series stdlib: Simplify buffer management in canonicalize | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Florian Weimer June 28, 2022, 11:45 a.m. UTC
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.

---
 stdlib/canonicalize.c | 113 ++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 63 deletions(-)

Comments

Siddhesh Poyarekar June 30, 2022, 3:51 a.m. UTC | #1
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);
>
diff mbox series

Patch

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);