Re: [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 4676 bytes --]

This email finishes the review of this proposed glibc patchset. (I 
didn't look at patch 4/5 for test cases.)

On 12/24/20 7:17 AM, Adhemerval Zanella wrote:

> +/* Check if BUFFER is using the internal buffer.  */
> +static __always_inline bool
> +scratch_buffer_using_internal (struct scratch_buffer *buffer)
> +{
> +  return buffer->data == buffer->__space.__c;
> +}
> +
> +/* Return the internal buffer from BUFFER if it is dynamic allocated,
> +   otherwise returns NULL.  Initializes the BUFFER if the internal
> +   dynamic buffer is returned.  */
> +static __always_inline void *
> +scratch_buffer_take_buffer (struct scratch_buffer *buffer)
> +{
> +  if (scratch_buffer_using_internal (buffer))
> +    return NULL;
> +
> +  void *r = buffer->data;
> +  scratch_buffer_init (buffer);
> +  return r;
> +}

This combination of functions is a little tricky. Instead, how about a 
single function that duplicates the scratch buffer on the heap, and 
frees the scratch buffer? Please see the attached proposed patch for 
Gnulib, which implements this idea. (I have not installed this into Gnulib.)

Also, shouldn't we merge the already-existing Gnulib scratch_buffer 
changes into glibc, along with this new change?

>   static idx_t
> +readlink_scratch_buffer (const char *path, struct scratch_buffer *buf)
> +{
> +  ssize_t r;
> +  while (true)
> +    {
> +      ptrdiff_t bufsize = buf->length;
> +      r = __readlink (path, buf->data, bufsize - 1);
> +      if (r < bufsize - 1)
> +	break;
> +      if (!scratch_buffer_grow (buf))
> +	return -1;
> +    }
> +  return r;
> +}

This function seems to exist because the proposed code calls readlink in 
two places. Current gnulib has been changed to call it in just one 
place, so there's less need to split out the function (the splitting 
complicates out-of-memory checking).

> -  scratch_buffer_init (rname_buf);
> -  char *rname_on_stack = rname_buf->data;
> ...
> +  scratch_buffer_init (&rname_buffer);
> +  char *rname = rname_buffer.data;

Doesn't this sort of thing have the potential to run into GCC bug 93644? 
That bug tends to be flaky; change platforms, or a few lines of code, 
and the problem recurs. Although it's just a bogus warning it cannot be 
turned off Gnulib has a GCC_LINT fix for this, which glibc could use 
simply with "#define GCC_LINT 1" in the "#ifdef _GLIBC" code.

> @@ -206,6 +219,20 @@ __realpath (const char *name, char *resolved)
>           /* nothing */;
>         else if (startlen == 2 && start[0] == '.' && start[1] == '.')
>           {
> +          if (!ISSLASH (dest[-1]))
> +            *dest++ = '/';
> +          *dest = '\0';
> +
> +	  ssize_t n = readlink_scratch_buffer (rname, &link_buffer);
> +          if (n < 0)
> +            {
> +              if (errno == ENOTDIR && dest[-1] == '/')
> +                dest[-1] = '\0';
> +	      if (errno == ENOMEM)
> +		goto error_nomem;
> +              if (errno != EINVAL)
> +                goto error;
> +            }

This can call readlink twice, once with trailing slash and once without. 
Better to call it just once.

> +	      char *buf = (char*) link_buffer.data;
>   
>                 buf[n] = '\0';
>   
> @@ -279,7 +296,7 @@ __realpath (const char *name, char *resolved)
>   
>                 /* Careful here, end may be a pointer into extra_buf... */
>                 memmove (&extra_buf[n], end, len + 1);
> -              name = end = memcpy (extra_buf, buf, n);
> +              name = end = memcpy (extra_buf, link_buffer.data, n);

If buf already equals link_buffer.data, no need for the patch to change 
buf to link_buffer.data.

> -          else if (! (startlen == 0
> -                      ? stat (rname, &st) == 0 || errno == EOVERFLOW
> -                      : errno == EINVAL))
> -            goto error;

I think current Gnulib addresses this issue in a different way, that 
doesn't involve the extra readlink calls.
>     if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && !prefix_len
>         && ISSLASH (*dest) && !ISSLASH (dest[1]))
>       dest++;
> +  *dest = '\0';
>     failed = false;
>   
>   error:
> -  *dest++ = '\0';

This looks dubious, as the error case also needs *dest to be '\0' and to 
increment dest (for when returning NULL when resolved != NULL).

Proposed patch to Gnulib attached. I hope this patch (along with what's 
already in Gnulib) addresses all the issues raised in your glibc 
patches, in the sense that the relevant files can be identical in Glibc 
and in Gnulib. I haven't installed this into Gnulib master on savannah.

[-- Attachment #2: 0001-canonicalize-simplify-via-scratch_buffer_dupfree.patch --]
[-- Type: text/x-patch, Size: 7665 bytes --]

From 5186fb6c9e2840ab921418bf73fe2662e200f89d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 24 Dec 2020 16:17:44 -0800
Subject: [PATCH] canonicalize: simplify via scratch_buffer_dupfree

* config/srclist.txt: Adjust accordingly.
* lib/canonicalize-lgpl.c (realpath_stk):
* lib/canonicalize.c (canonicalize_filename_mode_stk):
Simplify by using scratch_buffer_dupfree.
* lib/malloc/scratch_buffer.h (scratch_buffer_dupfree): New function.
* lib/malloc/scratch_buffer_dupfree.c: New file.
* modules/scratch_buffer (Files, Depends-on):
Add malloc/scratch_buffer_dupfree.c.
---
 ChangeLog                           | 10 +++++++
 config/srclist.txt                  |  3 ++-
 lib/canonicalize-lgpl.c             | 20 ++++----------
 lib/canonicalize.c                  |  9 +++----
 lib/malloc/scratch_buffer.h         | 16 +++++++++++
 lib/malloc/scratch_buffer_dupfree.c | 41 +++++++++++++++++++++++++++++
 modules/scratch_buffer              |  4 ++-
 7 files changed, 81 insertions(+), 22 deletions(-)
 create mode 100644 lib/malloc/scratch_buffer_dupfree.c

diff --git a/ChangeLog b/ChangeLog
index 30be929b5..6b3ca8b5c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,15 @@
 2020-12-24  Paul Eggert  <eggert@cs.ucla.edu>
 
+	canonicalize: simplify via scratch_buffer_dupfree
+	* config/srclist.txt: Adjust accordingly.
+	* lib/canonicalize-lgpl.c (realpath_stk):
+	* lib/canonicalize.c (canonicalize_filename_mode_stk):
+	Simplify by using scratch_buffer_dupfree.
+	* lib/malloc/scratch_buffer.h (scratch_buffer_dupfree): New function.
+	* lib/malloc/scratch_buffer_dupfree.c: New file.
+	* modules/scratch_buffer (Files, Depends-on):
+	Add malloc/scratch_buffer_dupfree.c.
+
 	canonicalize-lgpl: merge proposed libc changes
 	This merges the changes proposed for glibc in:
 	https://sourceware.org/pipermail/libc-alpha/2020-December/121085.html
diff --git a/config/srclist.txt b/config/srclist.txt
index f33b1353f..3956082c8 100644
--- a/config/srclist.txt
+++ b/config/srclist.txt
@@ -49,7 +49,8 @@ $GNUORG Copyright/request-assign.future		doc/Copyright
 $GNUORG Copyright/request-assign.program	doc/Copyright
 $GNUORG Copyright/request-disclaim.changes	doc/Copyright
 
-$LIBCSRC include/scratch_buffer.h	lib/malloc
+#$LIBCSRC include/scratch_buffer.h	lib/malloc
+#$LIBCSRC malloc/scratch_buffer_dupfree.c	lib/malloc
 $LIBCSRC malloc/scratch_buffer_grow.c	lib/malloc
 $LIBCSRC malloc/scratch_buffer_grow_preserve.c	lib/malloc
 $LIBCSRC malloc/scratch_buffer_set_array_size.c	lib/malloc
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 8c8f98abc..b5f51122b 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -404,24 +404,14 @@ error:
 error_nomem:
   scratch_buffer_free (&extra_buffer);
   scratch_buffer_free (&link_buffer);
-  if (failed || rname == resolved)
-    scratch_buffer_free (rname_buf);
-
-  if (failed)
-    return NULL;
 
-  if (rname == resolved)
-    return rname;
-  idx_t rname_size = dest - rname;
-  if (rname == rname_on_stack)
+  if (failed || rname == resolved)
     {
-      rname = malloc (rname_size);
-      if (rname == NULL)
-        return NULL;
-      return memcpy (rname, rname_on_stack, rname_size);
+      scratch_buffer_free (rname_buf);
+      return failed ? NULL : resolved;
     }
-  char *result = realloc (rname, rname_size);
-  return result != NULL ? result : rname;
+
+  return scratch_buffer_dupfree (rname_buf, dest - rname);
 }
 
 /* Return the canonical absolute name of file NAME.  A canonical name
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index 2c88335bf..366b658a2 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -463,11 +463,10 @@ error:
       return NULL;
     }
 
-  idx_t rname_size = dest - rname;
-  if (rname == rname_on_stack)
-    return xmemdup (rname, rname_size);
-  char *result = realloc (rname, rname_size);
-  return result != NULL ? result : rname;
+  char *result = scratch_buffer_dupfree (rname_buf, dest - rname);
+  if (!result)
+    xalloc_die ();
+  return result;
 }
 
 /* Return the canonical absolute name of file NAME, while treating
diff --git a/lib/malloc/scratch_buffer.h b/lib/malloc/scratch_buffer.h
index c39da7862..48d651b41 100644
--- a/lib/malloc/scratch_buffer.h
+++ b/lib/malloc/scratch_buffer.h
@@ -132,4 +132,20 @@ scratch_buffer_set_array_size (struct scratch_buffer *buffer,
 			 (buffer, nelem, size));
 }
 
+/* Return a copy of *BUFFER's first SIZE bytes as a heap-allocated block,
+   deallocating *BUFFER if it was heap-allocated.  SIZE must be at
+   most *BUFFER's size.  Return NULL (setting errno) on memory
+   exhaustion.  */
+void *__libc_scratch_buffer_dupfree (struct scratch_buffer *buffer,
+                                     size_t size);
+libc_hidden_proto (__libc_scratch_buffer_dupfree)
+
+/* Alias for __libc_scratch_dupfree.  */
+static __always_inline void *
+scratch_buffer_dupfree (struct scratch_buffer *buffer, size_t size)
+{
+  void *r = __libc_scratch_buffer_dupfree (buffer, size);
+  return __glibc_likely (r != NULL) ? r : NULL;
+}
+
 #endif /* _SCRATCH_BUFFER_H */
diff --git a/lib/malloc/scratch_buffer_dupfree.c b/lib/malloc/scratch_buffer_dupfree.c
new file mode 100644
index 000000000..5561e99b0
--- /dev/null
+++ b/lib/malloc/scratch_buffer_dupfree.c
@@ -0,0 +1,41 @@
+/* Variable-sized buffer with on-stack default allocation.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _LIBC
+# include <libc-config.h>
+#endif
+
+#include <scratch_buffer.h>
+#include <string.h>
+
+void *
+__libc_scratch_buffer_dupfree (struct scratch_buffer *buffer, size_t size)
+{
+  void *data = buffer->data;
+  if (data == buffer->__space.__c)
+    {
+      void *copy = malloc (size);
+      return copy != NULL ? memcpy (copy, data, size) : NULL;
+    }
+  else
+    {
+      void *copy = realloc (data, size);
+      return copy != NULL ? copy : data;
+    }
+}
+libc_hidden_def (__libc_scratch_buffer_dupfree)
diff --git a/modules/scratch_buffer b/modules/scratch_buffer
index 4f9a72581..7eedae7cc 100644
--- a/modules/scratch_buffer
+++ b/modules/scratch_buffer
@@ -4,6 +4,7 @@ Variable-sized buffer with on-stack default allocation.
 Files:
 lib/scratch_buffer.h
 lib/malloc/scratch_buffer.h
+lib/malloc/scratch_buffer_dupfree.c
 lib/malloc/scratch_buffer_grow.c
 lib/malloc/scratch_buffer_grow_preserve.c
 lib/malloc/scratch_buffer_set_array_size.c
@@ -17,7 +18,8 @@ stddef
 configure.ac:
 
 Makefile.am:
-lib_SOURCES += malloc/scratch_buffer_grow.c \
+lib_SOURCES += malloc/scratch_buffer_dupfree.c \
+               malloc/scratch_buffer_grow.c \
                malloc/scratch_buffer_grow_preserve.c \
                malloc/scratch_buffer_set_array_size.c
 
-- 
2.27.0