unofficial mirror of libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Fix multiple realpath issues
@ 2020-12-24 15:16 Adhemerval Zanella via Libc-alpha
  2020-12-24 15:16 ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-24 15:16 UTC (permalink / raw
  To: libc-alpha, Paul Eggert; +Cc: bug-gnulib

This is an updated version of my previous set to fix multiple realpath
implementation issues [1].

For this version I used the most recent gnulib version to sync, since
it constains multiple issues and it fixes glibc BZ #10635, BZ #26592,
and BZ #26241.  However, the gnulib version shows some regressions
on stdlib/tst-canon.c which I fixed on last patch (which also fixes
glibc BZ #24970).

This idea is to either apply this whole set or work with gnulib to
fix the glibc regression on their side and sync a fully working
implementation back to glibc.

[1] https://patchwork.sourceware.org/project/glibc/list/?series=1062

*** BLURB HERE ***

Adhemerval Zanella (5):
  stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ
    #26241]
  Import idx.h from gnulib
  Import filename.h from gnulib
  stdlib: Add testcase fro BZ #26241
  stdlib: Remove lstat usage from realpath [BZ #24970]

 include/filename.h                            | 110 +++++
 include/idx.h                                 | 113 +++++
 include/scratch_buffer.h                      |  21 +
 stdlib/Makefile                               |   3 +-
 stdlib/canonicalize.c                         | 467 ++++++++++++------
 stdlib/tst-canon-bz26341.c                    | 108 ++++
 support/support_set_small_thread_stack_size.c |  12 +-
 support/xthread.h                             |   2 +
 8 files changed, 676 insertions(+), 160 deletions(-)
 create mode 100644 include/filename.h
 create mode 100644 include/idx.h
 create mode 100644 stdlib/tst-canon-bz26341.c

-- 
2.25.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241]
  2020-12-24 15:16 [PATCH 0/5] Fix multiple realpath issues Adhemerval Zanella via Libc-alpha
@ 2020-12-24 15:16 ` Adhemerval Zanella via Libc-alpha
  2020-12-24 22:45   ` Paul Eggert
  2020-12-24 15:16 ` [PATCH 2/5] Import idx.h from gnulib Adhemerval Zanella via Libc-alpha
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-24 15:16 UTC (permalink / raw
  To: libc-alpha, Paul Eggert; +Cc: bug-gnulib

It sync with gnulib version d9c121346 with the following difference
require fix a glibc build:

--- ../../gnulib/gnulib-lib/lib/canonicalize-lgpl.c
+++ stdlib/canonicalize.c
@@ -46,7 +46,7 @@
 # define FILE_SYSTEM_PREFIX_LEN(name) 0
 # define IS_ABSOLUTE_FILE_NAME(name) ISSLASH(*(name))
 # define ISSLASH(c) ((c) == '/')
-# define freea(p) ((void) (p))
+# define FUNC_REALPATH_WORKS 1
 #else
 # define __canonicalize_file_name canonicalize_file_name
 # define __realpath realpath
@@ -270,7 +270,7 @@
               buf[n] = '\0';

               char *extra_buf = extra_buffer.data;
-              idx_t end_idx;
+              idx_t end_idx = 0;
               if (end_in_extra_buffer)
                 end_idx = end - extra_buf;
               idx_t len = strlen (end);

It shows the following testcase failures:

FAIL: stdlib/test-canon
azanella@birita:~/Projects/glibc/build/x86_64-linux-gnu$ cat stdlib/test-canon.out
stdlib/test-canon: flunked test 30 (expected resolved `./doesExist/someFile', got `[builddir]/stdlib/doesExist/someFile/')
stdlib/test-canon: flunked test 31 (expected `NULL', got `[builddir]/stdlib/doesExist')
2 errors.

(the [builddir] the build directory I used)

This regression will be fixed in the last commit of this set.

The sync also fixes BZ#26592 and BZ#26241.
---
 stdlib/canonicalize.c | 456 +++++++++++++++++++++++++++---------------
 1 file changed, 297 insertions(+), 159 deletions(-)

diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 3fcb399a5d..66685f4526 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -16,43 +16,126 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <assert.h>
+#ifndef _LIBC
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   optimizes away the name == NULL test below.  */
+# define _GL_ARG_NONNULL(params)
+
+# define _GL_USE_STDLIB_ALLOC 1
+# include <libc-config.h>
+#endif
+
+/* Specification.  */
 #include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include <limits.h>
-#include <sys/stat.h>
+
 #include <errno.h>
+#include <limits.h>
+#include <stdbool.h>
 #include <stddef.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include <scratch_buffer.h>
+
+#ifdef _LIBC
+# include <eloop-threshold.h>
+# include <shlib-compat.h>
+typedef ptrdiff_t idx_t;
+# define IDX_MAX PTRDIFF_MAX
+# define FILE_SYSTEM_PREFIX_LEN(name) 0
+# define IS_ABSOLUTE_FILE_NAME(name) ISSLASH(*(name))
+# define ISSLASH(c) ((c) == '/')
+# define FUNC_REALPATH_WORKS 1
+#else
+# define __canonicalize_file_name canonicalize_file_name
+# define __realpath realpath
+# include "idx.h"
+# include "pathmax.h"
+# include "filename.h"
+# if defined _WIN32 && !defined __CYGWIN__
+#  define __getcwd _getcwd
+# elif HAVE_GETCWD
+#  if IN_RELOCWRAPPER
+    /* When building the relocatable program wrapper, use the system's getcwd
+       function, not the gnulib override, otherwise we would get a link error.
+     */
+#   undef getcwd
+#  endif
+#  if defined VMS && !defined getcwd
+    /* We want the directory in Unix syntax, not in VMS syntax.
+       The gnulib override of 'getcwd' takes 2 arguments; the original VMS
+       'getcwd' takes 3 arguments.  */
+#   define __getcwd(buf, max) getcwd (buf, max, 0)
+#  else
+#   define __getcwd getcwd
+#  endif
+# else
+#  define __getcwd(buf, max) getwd (buf)
+# endif
+# define __mempcpy mempcpy
+# define __pathconf pathconf
+# define __rawmemchr rawmemchr
+# define __readlink readlink
+# ifndef MAXSYMLINKS
+#  ifdef SYMLOOP_MAX
+#   define MAXSYMLINKS SYMLOOP_MAX
+#  else
+#   define MAXSYMLINKS 20
+#  endif
+# endif
+# define __eloop_threshold() MAXSYMLINKS
+#endif
+
+#ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
+# define DOUBLE_SLASH_IS_DISTINCT_ROOT 0
+#endif
+
+#if !FUNC_REALPATH_WORKS || defined _LIBC
 
-#include <eloop-threshold.h>
-#include <shlib-compat.h>
+static idx_t
+get_path_max (void)
+{
+# ifdef PATH_MAX
+  long int path_max = PATH_MAX;
+# else
+  /* The caller invoked realpath with a null RESOLVED, even though
+     PATH_MAX is not defined as a constant.  The glibc manual says
+     programs should not do this, and POSIX says the behavior is undefined.
+     Historically, glibc here used the result of pathconf, or 1024 if that
+     failed; stay consistent with this (dubious) historical practice.  */
+  int err = errno;
+  long int path_max = __pathconf ("/", _PC_PATH_MAX);
+  __set_errno (err);
+# endif
+  return path_max < 0 ? 1024 : path_max <= IDX_MAX ? path_max : IDX_MAX;
+}
 
 /* Return the canonical absolute name of file NAME.  A canonical name
-   does not contain any `.', `..' components nor any repeated path
-   separators ('/') or symlinks.  All path components must exist.  If
+   does not contain any ".", ".." components nor any repeated file name
+   separators ('/') or symlinks.  All file name components must exist.  If
    RESOLVED is null, the result is malloc'd; otherwise, if the
-   canonical name is PATH_MAX chars or more, returns null with `errno'
+   canonical name is PATH_MAX chars or more, returns null with 'errno'
    set to ENAMETOOLONG; if the name fits in fewer than PATH_MAX chars,
    returns the name in RESOLVED.  If the name cannot be resolved and
-   RESOLVED is non-NULL, it contains the path of the first component
-   that cannot be resolved.  If the path can be resolved, RESOLVED
+   RESOLVED is non-NULL, it contains the name of the first component
+   that cannot be resolved.  If the name can be resolved, RESOLVED
    holds the same value as the value returned.  */
 
 char *
 __realpath (const char *name, char *resolved)
 {
-  char *rpath, *dest, *extra_buf = NULL;
-  const char *start, *end, *rpath_limit;
-  long int path_max;
+  char *dest;
+  char const *start;
+  char const *end;
   int num_links = 0;
 
   if (name == NULL)
     {
       /* As per Single Unix Specification V2 we must return an error if
-	 either parameter is a null pointer.  We extend this to allow
-	 the RESOLVED parameter to be NULL in case the we are expected to
-	 allocate the room for the return value.  */
+         either parameter is a null pointer.  We extend this to allow
+         the RESOLVED parameter to be NULL in case the we are expected to
+         allocate the room for the return value.  */
       __set_errno (EINVAL);
       return NULL;
     }
@@ -60,166 +143,221 @@ __realpath (const char *name, char *resolved)
   if (name[0] == '\0')
     {
       /* As per Single Unix Specification V2 we must return an error if
-	 the name argument points to an empty string.  */
+         the name argument points to an empty string.  */
       __set_errno (ENOENT);
       return NULL;
     }
 
-#ifdef PATH_MAX
-  path_max = PATH_MAX;
-#else
-  path_max = __pathconf (name, _PC_PATH_MAX);
-  if (path_max <= 0)
-    path_max = 1024;
-#endif
-
-  if (resolved == NULL)
-    {
-      rpath = malloc (path_max);
-      if (rpath == NULL)
-	return NULL;
-    }
-  else
-    rpath = resolved;
-  rpath_limit = rpath + path_max;
-
-  if (name[0] != '/')
+  struct scratch_buffer extra_buffer, link_buffer;
+  struct scratch_buffer rname_buffer;
+  struct scratch_buffer *rname_buf = &rname_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;
+  bool end_in_extra_buffer = false;
+  bool failed = true;
+
+  /* This is always zero for Posix hosts, but can be 2 for MS-Windows
+     and MS-DOS X:/foo/bar file names.  */
+  idx_t prefix_len = FILE_SYSTEM_PREFIX_LEN (name);
+
+  if (!IS_ABSOLUTE_FILE_NAME (name))
     {
-      if (!__getcwd (rpath, path_max))
-	{
-	  rpath[0] = '\0';
-	  goto error;
-	}
-      dest = __rawmemchr (rpath, '\0');
+      while (!__getcwd (rname, rname_buf->length))
+        {
+          if (errno != ERANGE)
+            {
+              dest = rname;
+              goto error;
+            }
+          if (!scratch_buffer_grow (rname_buf))
+            goto error_nomem;
+          rname = rname_buf->data;
+        }
+      dest = __rawmemchr (rname, '\0');
+      start = name;
+      prefix_len = FILE_SYSTEM_PREFIX_LEN (rname);
     }
   else
     {
-      rpath[0] = '/';
-      dest = rpath + 1;
+      dest = __mempcpy (rname, name, prefix_len);
+      *dest++ = '/';
+      if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+        {
+          if (prefix_len == 0 /* implies ISSLASH (name[0]) */
+              && ISSLASH (name[1]) && !ISSLASH (name[2]))
+            *dest++ = '/';
+          *dest = '\0';
+        }
+      start = name + prefix_len;
     }
 
-  for (start = end = name; *start; start = end)
+  for ( ; *start; start = end)
     {
-      struct stat64 st;
-      int n;
-
-      /* Skip sequence of multiple path-separators.  */
-      while (*start == '/')
-	++start;
-
-      /* Find end of path component.  */
-      for (end = start; *end && *end != '/'; ++end)
-	/* Nothing.  */;
-
-      if (end - start == 0)
-	break;
-      else if (end - start == 1 && start[0] == '.')
-	/* nothing */;
-      else if (end - start == 2 && start[0] == '.' && start[1] == '.')
-	{
-	  /* Back up to previous component, ignore if at root already.  */
-	  if (dest > rpath + 1)
-	    while ((--dest)[-1] != '/');
-	}
+      /* Skip sequence of multiple file name separators.  */
+      while (ISSLASH (*start))
+        ++start;
+
+      /* Find end of component.  */
+      for (end = start; *end && !ISSLASH (*end); ++end)
+        /* Nothing.  */;
+
+      /* Length of this file name component; it can be zero if a file
+         name ends in '/'.  */
+      idx_t startlen = end - start;
+
+      if (startlen == 1 && start[0] == '.')
+        /* nothing */;
+      else if (startlen == 2 && start[0] == '.' && start[1] == '.')
+        {
+          /* Back up to previous component, ignore if at root already.  */
+          if (dest > rname + prefix_len + 1)
+            for (--dest; dest > rname && !ISSLASH (dest[-1]); --dest)
+              continue;
+          if (DOUBLE_SLASH_IS_DISTINCT_ROOT
+              && dest == rname + 1 && !prefix_len
+              && ISSLASH (*dest) && !ISSLASH (dest[1]))
+            dest++;
+        }
       else
-	{
-	  size_t new_size;
-
-	  if (dest[-1] != '/')
-	    *dest++ = '/';
-
-	  if (dest + (end - start) >= rpath_limit)
-	    {
-	      ptrdiff_t dest_offset = dest - rpath;
-	      char *new_rpath;
-
-	      if (resolved)
-		{
-		  __set_errno (ENAMETOOLONG);
-		  if (dest > rpath + 1)
-		    dest--;
-		  *dest = '\0';
-		  goto error;
-		}
-	      new_size = rpath_limit - rpath;
-	      if (end - start + 1 > path_max)
-		new_size += end - start + 1;
-	      else
-		new_size += path_max;
-	      new_rpath = (char *) realloc (rpath, new_size);
-	      if (new_rpath == NULL)
-		goto error;
-	      rpath = new_rpath;
-	      rpath_limit = rpath + new_size;
-
-	      dest = rpath + dest_offset;
-	    }
-
-	  dest = __mempcpy (dest, start, end - start);
-	  *dest = '\0';
-
-	  if (__lstat64 (rpath, &st) < 0)
-	    goto error;
-
-	  if (S_ISLNK (st.st_mode))
-	    {
-	      char *buf = __alloca (path_max);
-	      size_t len;
-
-	      if (++num_links > __eloop_threshold ())
-		{
-		  __set_errno (ELOOP);
-		  goto error;
-		}
-
-	      n = __readlink (rpath, buf, path_max - 1);
-	      if (n < 0)
-		goto error;
-	      buf[n] = '\0';
-
-	      if (!extra_buf)
-		extra_buf = __alloca (path_max);
-
-	      len = strlen (end);
-	      if (path_max - n <= len)
-		{
-		  __set_errno (ENAMETOOLONG);
-		  goto error;
-		}
-
-	      /* Careful here, end may be a pointer into extra_buf... */
-	      memmove (&extra_buf[n], end, len + 1);
-	      name = end = memcpy (extra_buf, buf, n);
-
-	      if (buf[0] == '/')
-		dest = rpath + 1;	/* It's an absolute symlink */
-	      else
-		/* Back up to previous component, ignore if at root already: */
-		if (dest > rpath + 1)
-		  while ((--dest)[-1] != '/');
-	    }
-	  else if (!S_ISDIR (st.st_mode) && *end != '\0')
-	    {
-	      __set_errno (ENOTDIR);
-	      goto error;
-	    }
-	}
+        {
+          if (!ISSLASH (dest[-1]))
+            *dest++ = '/';
+
+          while (rname + rname_buf->length - dest <= startlen)
+            {
+              idx_t dest_offset = dest - rname;
+              if (!scratch_buffer_grow_preserve (rname_buf))
+                goto error_nomem;
+              rname = rname_buf->data;
+              dest = rname + dest_offset;
+            }
+
+          dest = __mempcpy (dest, start, startlen);
+          *dest = '\0';
+
+          /* If STARTLEN == 0, RNAME ends in '/'; use stat rather than
+             readlink, because readlink might fail with EINVAL without
+             checking whether RNAME sans '/' is valid.  */
+          struct stat st;
+          char *buf = NULL;
+          ssize_t n;
+          if (startlen != 0)
+            {
+              while (true)
+                {
+                  buf = link_buffer.data;
+                  idx_t bufsize = link_buffer.length;
+                  n = __readlink (rname, buf, bufsize - 1);
+                  if (n < bufsize - 1)
+                    break;
+                  if (!scratch_buffer_grow (&link_buffer))
+                    goto error_nomem;
+                }
+              if (n < 0)
+                buf = NULL;
+            }
+          if (buf)
+            {
+              if (++num_links > __eloop_threshold ())
+                {
+                  __set_errno (ELOOP);
+                  goto error;
+                }
+
+              buf[n] = '\0';
+
+              char *extra_buf = extra_buffer.data;
+              idx_t end_idx = 0;
+              if (end_in_extra_buffer)
+                end_idx = end - extra_buf;
+              idx_t len = strlen (end);
+              while (extra_buffer.length <= len + n)
+                {
+                  if (!scratch_buffer_grow_preserve (&extra_buffer))
+                    goto error_nomem;
+                  extra_buf = extra_buffer.data;
+                }
+              if (end_in_extra_buffer)
+                end = extra_buf + end_idx;
+
+              /* Careful here, end may be a pointer into extra_buf... */
+              memmove (&extra_buf[n], end, len + 1);
+              name = end = memcpy (extra_buf, buf, n);
+              end_in_extra_buffer = true;
+
+              if (IS_ABSOLUTE_FILE_NAME (buf))
+                {
+                  idx_t pfxlen = FILE_SYSTEM_PREFIX_LEN (buf);
+
+                  dest = __mempcpy (rname, buf, pfxlen);
+                  *dest++ = '/'; /* It's an absolute symlink */
+                  if (DOUBLE_SLASH_IS_DISTINCT_ROOT)
+                    {
+                      if (ISSLASH (buf[1]) && !ISSLASH (buf[2]) && !pfxlen)
+                        *dest++ = '/';
+                      *dest = '\0';
+                    }
+                  /* Install the new prefix to be in effect hereafter.  */
+                  prefix_len = pfxlen;
+                }
+              else
+                {
+                  /* Back up to previous component, ignore if at root
+                     already: */
+                  if (dest > rname + prefix_len + 1)
+                    for (--dest; dest > rname && !ISSLASH (dest[-1]); --dest)
+                      continue;
+                  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1
+                      && ISSLASH (*dest) && !ISSLASH (dest[1]) && !prefix_len)
+                    dest++;
+                }
+            }
+          else if (! (startlen == 0
+                      ? stat (rname, &st) == 0 || errno == EOVERFLOW
+                      : errno == EINVAL))
+            goto error;
+        }
     }
-  if (dest > rpath + 1 && dest[-1] == '/')
+  if (dest > rname + prefix_len + 1 && ISSLASH (dest[-1]))
     --dest;
-  *dest = '\0';
-
-  assert (resolved == NULL || resolved == rpath);
-  return rpath;
+  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && !prefix_len
+      && ISSLASH (*dest) && !ISSLASH (dest[1]))
+    dest++;
+  failed = false;
 
 error:
-  assert (resolved == NULL || resolved == rpath);
-  if (resolved == NULL)
-    free (rpath);
-  return NULL;
+  *dest++ = '\0';
+  if (resolved != NULL && dest - rname <= get_path_max ())
+    rname = strcpy (resolved, rname);
+
+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)
+    {
+      rname = malloc (rname_size);
+      if (rname == NULL)
+        return NULL;
+      return memcpy (rname, rname_on_stack, rname_size);
+    }
+  char *result = realloc (rname, rname_size);
+  return result != NULL ? result : rname;
 }
 libc_hidden_def (__realpath)
 versioned_symbol (libc, __realpath, realpath, GLIBC_2_3);
+#endif /* !FUNC_REALPATH_WORKS || defined _LIBC */
 
 
 #if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_3)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/5] Import idx.h from gnulib
  2020-12-24 15:16 [PATCH 0/5] Fix multiple realpath issues Adhemerval Zanella via Libc-alpha
  2020-12-24 15:16 ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
@ 2020-12-24 15:16 ` Adhemerval Zanella via Libc-alpha
  2020-12-24 22:53   ` Paul Eggert
                     ` (2 more replies)
  2020-12-24 15:16 ` [PATCH 3/5] Import filename.h " Adhemerval Zanella via Libc-alpha
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-24 15:16 UTC (permalink / raw
  To: libc-alpha, Paul Eggert; +Cc: bug-gnulib

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=ojwedtwnpqxshmrr, Size: 5617 bytes --]

And use to simplify stdlib/canonicalize.c implementation.
---
 include/idx.h         | 113 ++++++++++++++++++++++++++++++++++++++++++
 stdlib/canonicalize.c |   5 +-
 2 files changed, 114 insertions(+), 4 deletions(-)
 create mode 100644 include/idx.h

diff --git a/include/idx.h b/include/idx.h
new file mode 100644
index 0000000000..ad7d31a2bc
--- /dev/null
+++ b/include/idx.h
@@ -0,0 +1,113 @@
+/* A type for indices and sizes.
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, see <https://www.gnu.org/licenses/>.  */
+
+#ifndef _IDX_H
+#define _IDX_H
+
+/* Get ptrdiff_t.  */
+#include <stddef.h>
+
+/* Get PTRDIFF_MAX.  */
+#include <stdint.h>
+
+/* The type 'idx_t' holds an (array) index or an (object) size.
+   Its implementation promotes to a signed integer type,
+   which can hold the values
+     0..2^63-1 (on 64-bit platforms) or
+     0..2^31-1 (on 32-bit platforms).
+
+   Why a signed integer type?
+
+     * Security: Signed types can be checked for overflow via
+       '-fsanitize=undefined', but unsigned types cannot.
+
+     * Comparisons without surprises: ISO C99 § 6.3.1.8 specifies a few
+       surprising results for comparisons, such as
+
+           (int) -3 < (unsigned long) 7  =>  false
+           (int) -3 < (unsigned int) 7   =>  false
+       and on 32-bit machines:
+           (long) -3 < (unsigned int) 7  =>  false
+
+       This is surprising because the natural comparison order is by
+       value in the realm of infinite-precision signed integers (ℤ).
+
+       The best way to get rid of such surprises is to use signed types
+       for numerical integer values, and use unsigned types only for
+       bit masks and enums.
+
+   Why not use 'size_t' directly?
+
+     * Because 'size_t' is an unsigned type, and a signed type is better.
+       See above.
+
+   Why not use 'ptrdiff_t' directly?
+
+     * Maintainability: When reading and modifying code, it helps to know that
+       a certain variable cannot have negative values.  For example, when you
+       have a loop
+
+         int n = ...;
+         for (int i = 0; i < n; i++) ...
+
+       or
+
+         ptrdiff_t n = ...;
+         for (ptrdiff_t i = 0; i < n; i++) ...
+
+       you have to ask yourself "what if n < 0?".  Whereas in
+
+         idx_t n = ...;
+         for (idx_t i = 0; i < n; i++) ...
+
+       you know that this case cannot happen.
+
+       Similarly, when a programmer writes
+
+         idx_t = ptr2 - ptr1;
+
+       there is an implied assertion that ptr1 and ptr2 point into the same
+       object and that ptr1 <= ptr2.
+
+     * Being future-proof: In the future, range types (integers which are
+       constrained to a certain range of values) may be added to C compilers
+       or to the C standard.  Several programming languages (Ada, Haskell,
+       Common Lisp, Pascal) already have range types.  Such range types may
+       help producing good code and good warnings.  The type 'idx_t' could
+       then be typedef'ed to a range type that is signed after promotion.  */
+
+/* In the future, idx_t could be typedef'ed to a signed range type.
+   The clang "extended integer types", supported in Clang 11 or newer
+   <https://clang.llvm.org/docs/LanguageExtensions.html#extended-integer-types>,
+   are a special case of range types.  However, these types don't support binary
+   operators with plain integer types (e.g. expressions such as x > 1).
+   Therefore, they don't behave like signed types (and not like unsigned types
+   either).  So, we cannot use them here.  */
+
+/* Use the signed type 'ptrdiff_t'.  */
+/* Note: ISO C does not mandate that 'size_t' and 'ptrdiff_t' have the same
+   size, but it is so on all platforms we have seen since 1990.  */
+typedef ptrdiff_t idx_t;
+
+/* IDX_MAX is the maximum value of an idx_t.  */
+#define IDX_MAX PTRDIFF_MAX
+
+/* So far no need has been found for an IDX_WIDTH macro.
+   Perhaps there should be another macro IDX_VALUE_BITS that does not
+   count the sign bit and is therefore one less than PTRDIFF_WIDTH.  */
+
+#endif /* _IDX_H */
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 66685f4526..4b3bd10a9d 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -35,14 +35,13 @@
 #include <string.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include <idx.h>
 
 #include <scratch_buffer.h>
 
 #ifdef _LIBC
 # include <eloop-threshold.h>
 # include <shlib-compat.h>
-typedef ptrdiff_t idx_t;
-# define IDX_MAX PTRDIFF_MAX
 # define FILE_SYSTEM_PREFIX_LEN(name) 0
 # define IS_ABSOLUTE_FILE_NAME(name) ISSLASH(*(name))
 # define ISSLASH(c) ((c) == '/')
@@ -50,7 +49,6 @@ typedef ptrdiff_t idx_t;
 #else
 # define __canonicalize_file_name canonicalize_file_name
 # define __realpath realpath
-# include "idx.h"
 # include "pathmax.h"
 # include "filename.h"
 # if defined _WIN32 && !defined __CYGWIN__
@@ -92,7 +90,6 @@ typedef ptrdiff_t idx_t;
 #endif
 
 #if !FUNC_REALPATH_WORKS || defined _LIBC
-
 static idx_t
 get_path_max (void)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/5] Import filename.h from gnulib
  2020-12-24 15:16 [PATCH 0/5] Fix multiple realpath issues Adhemerval Zanella via Libc-alpha
  2020-12-24 15:16 ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
  2020-12-24 15:16 ` [PATCH 2/5] Import idx.h from gnulib Adhemerval Zanella via Libc-alpha
@ 2020-12-24 15:16 ` Adhemerval Zanella via Libc-alpha
  2020-12-31 23:13   ` Joseph Myers
  2020-12-24 15:17 ` [PATCH 4/5] stdlib: Add testcase fro BZ #26241 Adhemerval Zanella via Libc-alpha
  2020-12-24 15:17 ` [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970] Adhemerval Zanella via Libc-alpha
  4 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-24 15:16 UTC (permalink / raw
  To: libc-alpha, Paul Eggert; +Cc: bug-gnulib

And use to simplify stdlib/canonicalize.c implementation.
---
 include/filename.h    | 110 ++++++++++++++++++++++++++++++++++++++++++
 stdlib/canonicalize.c |   5 +-
 2 files changed, 111 insertions(+), 4 deletions(-)
 create mode 100644 include/filename.h

diff --git a/include/filename.h b/include/filename.h
new file mode 100644
index 0000000000..4598fb1d63
--- /dev/null
+++ b/include/filename.h
@@ -0,0 +1,110 @@
+/* Basic filename support macros.
+   Copyright (C) 2001-2004, 2007-2020 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+/* From Paul Eggert and Jim Meyering.  */
+
+#ifndef _FILENAME_H
+#define _FILENAME_H
+
+#include <string.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+
+/* Filename support.
+   ISSLASH(C)                  tests whether C is a directory separator
+                               character.
+   HAS_DEVICE(Filename)        tests whether Filename contains a device
+                               specification.
+   FILE_SYSTEM_PREFIX_LEN(Filename)  length of the device specification
+                                     at the beginning of Filename,
+                                     index of the part consisting of
+                                     alternating components and slashes.
+   FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE
+                               1 when a non-empty device specification
+                               can be followed by an empty or relative
+                               part,
+                               0 when a non-empty device specification
+                               must be followed by a slash,
+                               0 when device specification don't exist.
+   IS_ABSOLUTE_FILE_NAME(Filename)
+                               tests whether Filename is independent of
+                               any notion of "current directory".
+   IS_RELATIVE_FILE_NAME(Filename)
+                               tests whether Filename may be concatenated
+                               to a directory filename.
+   Note: On native Windows, OS/2, DOS, "c:" is neither an absolute nor a
+   relative file name!
+   IS_FILE_NAME_WITH_DIR(Filename)  tests whether Filename contains a device
+                                    or directory specification.
+ */
+#if defined _WIN32 || defined __CYGWIN__ \
+    || defined __EMX__ || defined __MSDOS__ || defined __DJGPP__
+  /* Native Windows, Cygwin, OS/2, DOS */
+# define ISSLASH(C) ((C) == '/' || (C) == '\\')
+  /* Internal macro: Tests whether a character is a drive letter.  */
+# define _IS_DRIVE_LETTER(C) \
+    (((C) >= 'A' && (C) <= 'Z') || ((C) >= 'a' && (C) <= 'z'))
+  /* Help the compiler optimizing it.  This assumes ASCII.  */
+# undef _IS_DRIVE_LETTER
+# define _IS_DRIVE_LETTER(C) \
+    (((unsigned int) (C) | ('a' - 'A')) - 'a' <= 'z' - 'a')
+# define HAS_DEVICE(Filename) \
+    (_IS_DRIVE_LETTER ((Filename)[0]) && (Filename)[1] == ':')
+# define FILE_SYSTEM_PREFIX_LEN(Filename) (HAS_DEVICE (Filename) ? 2 : 0)
+# ifdef __CYGWIN__
+#  define FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE 0
+# else
+   /* On native Windows, OS/2, DOS, the system has the notion of a
+      "current directory" on each drive.  */
+#  define FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE 1
+# endif
+# if FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE
+#  define IS_ABSOLUTE_FILE_NAME(Filename) \
+     ISSLASH ((Filename)[FILE_SYSTEM_PREFIX_LEN (Filename)])
+# else
+#  define IS_ABSOLUTE_FILE_NAME(Filename) \
+     (ISSLASH ((Filename)[0]) || HAS_DEVICE (Filename))
+# endif
+# define IS_RELATIVE_FILE_NAME(Filename) \
+    (! (ISSLASH ((Filename)[0]) || HAS_DEVICE (Filename)))
+# define IS_FILE_NAME_WITH_DIR(Filename) \
+    (strchr ((Filename), '/') != NULL || strchr ((Filename), '\\') != NULL \
+     || HAS_DEVICE (Filename))
+#else
+  /* Unix */
+# define ISSLASH(C) ((C) == '/')
+# define HAS_DEVICE(Filename) ((void) (Filename), 0)
+# define FILE_SYSTEM_PREFIX_LEN(Filename) ((void) (Filename), 0)
+# define FILE_SYSTEM_DRIVE_PREFIX_CAN_BE_RELATIVE 0
+# define IS_ABSOLUTE_FILE_NAME(Filename) ISSLASH ((Filename)[0])
+# define IS_RELATIVE_FILE_NAME(Filename) (! ISSLASH ((Filename)[0]))
+# define IS_FILE_NAME_WITH_DIR(Filename) (strchr ((Filename), '/') != NULL)
+#endif
+
+/* Deprecated macros.  For backward compatibility with old users of the
+   'filename' module.  */
+#define IS_ABSOLUTE_PATH IS_ABSOLUTE_FILE_NAME
+#define IS_PATH_WITH_DIR IS_FILE_NAME_WITH_DIR
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _FILENAME_H */
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 4b3bd10a9d..9111d92a4c 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -36,21 +36,18 @@
 #include <sys/stat.h>
 #include <unistd.h>
 #include <idx.h>
+#include <filename.h>
 
 #include <scratch_buffer.h>
 
 #ifdef _LIBC
 # include <eloop-threshold.h>
 # include <shlib-compat.h>
-# define FILE_SYSTEM_PREFIX_LEN(name) 0
-# define IS_ABSOLUTE_FILE_NAME(name) ISSLASH(*(name))
-# define ISSLASH(c) ((c) == '/')
 # define FUNC_REALPATH_WORKS 1
 #else
 # define __canonicalize_file_name canonicalize_file_name
 # define __realpath realpath
 # include "pathmax.h"
-# include "filename.h"
 # if defined _WIN32 && !defined __CYGWIN__
 #  define __getcwd _getcwd
 # elif HAVE_GETCWD
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/5] stdlib: Add testcase fro BZ #26241
  2020-12-24 15:16 [PATCH 0/5] Fix multiple realpath issues Adhemerval Zanella via Libc-alpha
                   ` (2 preceding siblings ...)
  2020-12-24 15:16 ` [PATCH 3/5] Import filename.h " Adhemerval Zanella via Libc-alpha
@ 2020-12-24 15:17 ` Adhemerval Zanella via Libc-alpha
  2020-12-24 15:17 ` [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970] Adhemerval Zanella via Libc-alpha
  4 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-24 15:17 UTC (permalink / raw
  To: libc-alpha, Paul Eggert; +Cc: bug-gnulib

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 stdlib/Makefile                               |   3 +-
 stdlib/tst-canon-bz26341.c                    | 108 ++++++++++++++++++
 support/support_set_small_thread_stack_size.c |  12 +-
 support/xthread.h                             |   2 +
 4 files changed, 121 insertions(+), 4 deletions(-)
 create mode 100644 stdlib/tst-canon-bz26341.c

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 29b7cd7071..6518d8993b 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -86,7 +86,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-makecontext-align test-bz22786 tst-strtod-nan-sign \
 		   tst-swapcontext1 tst-setcontext4 tst-setcontext5 \
 		   tst-setcontext6 tst-setcontext7 tst-setcontext8 \
-		   tst-setcontext9 tst-bz20544
+		   tst-setcontext9 tst-bz20544 tst-canon-bz26341
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
@@ -101,6 +101,7 @@ LDLIBS-test-atexit-race = $(shared-thread-library)
 LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
 LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
 LDLIBS-test-on_exit-race = $(shared-thread-library)
+LDLIBS-tst-canon-bz26341 = $(shared-thread-library)
 
 LDLIBS-test-dlclose-exit-race = $(shared-thread-library) $(libdl)
 LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic)
diff --git a/stdlib/tst-canon-bz26341.c b/stdlib/tst-canon-bz26341.c
new file mode 100644
index 0000000000..63474bddaa
--- /dev/null
+++ b/stdlib/tst-canon-bz26341.c
@@ -0,0 +1,108 @@
+/* Check if realpath does not consume extra stack space based on symlink
+   existance in the path (BZ #26341)
+   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/>.  */
+
+#include <stdlib.h>
+#include <string.h>
+#include <sys/param.h>
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xunistd.h>
+#include <support/xthread.h>
+
+static char *filename;
+static size_t filenamelen;
+static char *linkname;
+
+static int
+maxsymlinks (void)
+{
+#ifdef MAXSYMLINKS
+  return MAXSYMLINKS;
+#else
+  long int sysconf_symloop_max = sysconf (_SC_SYMLOOP_MAX);
+  return sysconf_symloop_max <= 0
+	 ? _POSIX_SYMLOOP_MAX
+	 : sysconf_symloop_max;
+#endif
+}
+
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
+static void
+create_link (void)
+{
+  int fd = create_temp_file ("tst-canon-bz26341", &filename);
+  TEST_VERIFY_EXIT (fd != -1);
+  xclose (fd);
+
+  char *prevlink = filename;
+  int maxlinks = maxsymlinks ();
+  for (int i = 0; i < maxlinks; i++)
+    {
+      linkname = xasprintf ("%s%d", filename, i);
+      xsymlink (prevlink, linkname);
+      add_temp_file (linkname);
+      prevlink = linkname;
+    }
+
+  filenamelen = strlen (filename);
+}
+
+static void *
+do_realpath (void *arg)
+{
+  /* Old implementation of realpath allocates a PATH_MAX using alloca
+     for each symlink in the path, leading to MAXSYMLINKS times PATH_MAX
+     maximum stack usage.
+     This stack allocations tries fill the thread allocated stack minus
+     both the thread (plus some slack) and the realpath (plus some slack).
+     If realpath uses more than 2 * PATH_MAX plus some slack it will trigger
+     a stackoverflow.  */
+
+  const size_t realpath_usage = 2 * PATH_MAX + 1024;
+  const size_t thread_usage = 1 * PATH_MAX + 1024;
+  size_t stack_size = support_small_thread_stack_size ()
+		      - realpath_usage - thread_usage;
+  char stack[stack_size];
+  char *resolved = stack + stack_size - thread_usage + 1024;
+
+  char *p = realpath (linkname, resolved);
+  TEST_VERIFY (p != NULL);
+  TEST_COMPARE_BLOB (resolved, filenamelen, filename, filenamelen);
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  create_link ();
+
+  pthread_t th = xpthread_create (support_small_stack_thread_attribute (),
+				  do_realpath, NULL);
+  xpthread_join (th);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/support/support_set_small_thread_stack_size.c b/support/support_set_small_thread_stack_size.c
index 69d66e97db..74a0e38a72 100644
--- a/support/support_set_small_thread_stack_size.c
+++ b/support/support_set_small_thread_stack_size.c
@@ -20,8 +20,8 @@
 #include <pthread.h>
 #include <support/xthread.h>
 
-void
-support_set_small_thread_stack_size (pthread_attr_t *attr)
+size_t
+support_small_thread_stack_size (void)
 {
   /* Some architectures have too small values for PTHREAD_STACK_MIN
      which cannot be used for creating threads.  Ensure that the stack
@@ -31,5 +31,11 @@ support_set_small_thread_stack_size (pthread_attr_t *attr)
   if (stack_size < PTHREAD_STACK_MIN)
     stack_size = PTHREAD_STACK_MIN;
 #endif
-  xpthread_attr_setstacksize (attr, stack_size);
+  return stack_size;
+}
+
+void
+support_set_small_thread_stack_size (pthread_attr_t *attr)
+{
+  xpthread_attr_setstacksize (attr, support_small_thread_stack_size ());
 }
diff --git a/support/xthread.h b/support/xthread.h
index 05f8d4a7d9..6ba2f5a18b 100644
--- a/support/xthread.h
+++ b/support/xthread.h
@@ -78,6 +78,8 @@ void xpthread_attr_setguardsize (pthread_attr_t *attr,
 /* Set the stack size in ATTR to a small value, but still large enough
    to cover most internal glibc stack usage.  */
 void support_set_small_thread_stack_size (pthread_attr_t *attr);
+/* Return the stack size used on support_set_small_thread_stack_size.  */
+size_t support_small_thread_stack_size (void);
 
 /* Return a pointer to a thread attribute which requests a small
    stack.  The caller must not free this pointer.  */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]
  2020-12-24 15:16 [PATCH 0/5] Fix multiple realpath issues Adhemerval Zanella via Libc-alpha
                   ` (3 preceding siblings ...)
  2020-12-24 15:17 ` [PATCH 4/5] stdlib: Add testcase fro BZ #26241 Adhemerval Zanella via Libc-alpha
@ 2020-12-24 15:17 ` Adhemerval Zanella via Libc-alpha
  2020-12-25  0:27   ` Paul Eggert
  4 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-24 15:17 UTC (permalink / raw
  To: libc-alpha, Paul Eggert; +Cc: bug-gnulib

The readlink already tells whether the file is a symlink, so there is
no need to call lstat to check it.  However for '..' it requires an
extra readlink check if the previous component can be really accessed,
otherwise the next iteration will check a possible valid path and end
early.  It should performance-wise acceptable and a gain over lstat,
afaik symlink should not update any inode information.

It also fixes the stdlib/test-canon issued from the gnulib sync.

Checked on x86_64-linux-gnu.
---
 include/scratch_buffer.h |  21 ++++++
 stdlib/canonicalize.c    | 139 +++++++++++++++++++++++----------------
 2 files changed, 102 insertions(+), 58 deletions(-)

diff --git a/include/scratch_buffer.h b/include/scratch_buffer.h
index c39da78629..7ae77e5160 100644
--- a/include/scratch_buffer.h
+++ b/include/scratch_buffer.h
@@ -132,4 +132,25 @@ scratch_buffer_set_array_size (struct scratch_buffer *buffer,
 			 (buffer, nelem, size));
 }
 
+/* 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;
+}
+
 #endif /* _SCRATCH_BUFFER_H */
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 9111d92a4c..78a06227d2 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -88,6 +88,21 @@
 
 #if !FUNC_REALPATH_WORKS || defined _LIBC
 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;
+}
+static idx_t
 get_path_max (void)
 {
 # ifdef PATH_MAX
@@ -144,12 +159,10 @@ __realpath (const char *name, char *resolved)
 
   struct scratch_buffer extra_buffer, link_buffer;
   struct scratch_buffer rname_buffer;
-  struct scratch_buffer *rname_buf = &rname_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;
+  scratch_buffer_init (&rname_buffer);
+  char *rname = rname_buffer.data;
   bool end_in_extra_buffer = false;
   bool failed = true;
 
@@ -159,16 +172,16 @@ __realpath (const char *name, char *resolved)
 
   if (!IS_ABSOLUTE_FILE_NAME (name))
     {
-      while (!__getcwd (rname, rname_buf->length))
+      while (!__getcwd (rname, rname_buffer.length))
         {
           if (errno != ERANGE)
             {
               dest = rname;
               goto error;
             }
-          if (!scratch_buffer_grow (rname_buf))
+          if (!scratch_buffer_grow (&rname_buffer))
             goto error_nomem;
-          rname = rname_buf->data;
+	  rname = rname_buffer.data;
         }
       dest = __rawmemchr (rname, '\0');
       start = name;
@@ -188,7 +201,7 @@ __realpath (const char *name, char *resolved)
       start = name + prefix_len;
     }
 
-  for ( ; *start; start = end)
+  for (end = start ; *start; start = end)
     {
       /* Skip sequence of multiple file name separators.  */
       while (ISSLASH (*start))
@@ -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;
+            }
           /* Back up to previous component, ignore if at root already.  */
           if (dest > rname + prefix_len + 1)
             for (--dest; dest > rname && !ISSLASH (dest[-1]); --dest)
@@ -220,46 +247,36 @@ __realpath (const char *name, char *resolved)
           if (!ISSLASH (dest[-1]))
             *dest++ = '/';
 
-          while (rname + rname_buf->length - dest <= startlen)
+          while (rname + rname_buffer.length - dest <= startlen)
             {
               idx_t dest_offset = dest - rname;
-              if (!scratch_buffer_grow_preserve (rname_buf))
+              if (!scratch_buffer_grow_preserve (&rname_buffer))
                 goto error_nomem;
-              rname = rname_buf->data;
+              rname = rname_buffer.data;
               dest = rname + dest_offset;
             }
 
           dest = __mempcpy (dest, start, startlen);
           *dest = '\0';
 
-          /* If STARTLEN == 0, RNAME ends in '/'; use stat rather than
-             readlink, because readlink might fail with EINVAL without
-             checking whether RNAME sans '/' is valid.  */
-          struct stat st;
-          char *buf = NULL;
-          ssize_t n;
-          if (startlen != 0)
+          ssize_t n = readlink_scratch_buffer (rname, &link_buffer);
+          if (n < 0)
             {
-              while (true)
-                {
-                  buf = link_buffer.data;
-                  idx_t bufsize = link_buffer.length;
-                  n = __readlink (rname, buf, bufsize - 1);
-                  if (n < bufsize - 1)
-                    break;
-                  if (!scratch_buffer_grow (&link_buffer))
-                    goto error_nomem;
-                }
-              if (n < 0)
-                buf = NULL;
+              if (errno == ENOTDIR && dest[-1] == '/')
+                dest[-1] = '\0';
+              if (errno == ENOMEM)
+                goto error_nomem;
+              if (errno != EINVAL)
+                goto error;
             }
-          if (buf)
+          else
             {
               if (++num_links > __eloop_threshold ())
                 {
                   __set_errno (ELOOP);
                   goto error;
                 }
+	      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);
               end_in_extra_buffer = true;
 
               if (IS_ABSOLUTE_FILE_NAME (buf))
@@ -309,10 +326,6 @@ __realpath (const char *name, char *resolved)
                     dest++;
                 }
             }
-          else if (! (startlen == 0
-                      ? stat (rname, &st) == 0 || errno == EOVERFLOW
-                      : errno == EINVAL))
-            goto error;
         }
     }
   if (dest > rname + prefix_len + 1 && ISSLASH (dest[-1]))
@@ -320,34 +333,44 @@ __realpath (const char *name, char *resolved)
   if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && !prefix_len
       && ISSLASH (*dest) && !ISSLASH (dest[1]))
     dest++;
+  *dest = '\0';
   failed = false;
 
 error:
-  *dest++ = '\0';
-  if (resolved != NULL && dest - rname <= get_path_max ())
-    rname = strcpy (resolved, rname);
+  if (resolved != NULL)
+    {
+      if (dest - rname <= get_path_max ())
+        rname = strcpy (resolved, rname);
+    }
+  else
+    {
+      if (rname == resolved)
+	return rname;
+
+      idx_t rname_size = dest - rname;
+      if (scratch_buffer_using_internal (&rname_buffer))
+        {
+          rname = malloc (rname_size + 1);
+	  if (rname != NULL)
+	    {
+              memcpy (rname, rname_buffer.data, rname_size);
+	      rname[rname_size] = '\0';
+	    }
+        }
+      else
+        {
+	  rname = scratch_buffer_take_buffer (&rname_buffer);
+	  char *result = realloc (rname, rname_size);
+	  if (result != NULL)
+	    rname = result;
+        }
+    }
 
 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)
-    {
-      rname = malloc (rname_size);
-      if (rname == NULL)
-        return NULL;
-      return memcpy (rname, rname_on_stack, rname_size);
-    }
-  char *result = realloc (rname, rname_size);
-  return result != NULL ? result : rname;
+  scratch_buffer_free (&extra_buffer);
+  scratch_buffer_free (&rname_buffer);
+  return failed ? NULL : rname;
 }
 libc_hidden_def (__realpath)
 versioned_symbol (libc, __realpath, realpath, GLIBC_2_3);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241]
  2020-12-24 15:16 ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
@ 2020-12-24 22:45   ` Paul Eggert
  2020-12-25  0:44     ` [PATCH 1/5] warnings in canonicalize.c Bruno Haible
  2020-12-28 12:53     ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
  0 siblings, 2 replies; 22+ messages in thread
From: Paul Eggert @ 2020-12-24 22:45 UTC (permalink / raw
  To: Adhemerval Zanella; +Cc: libc-alpha, bug-gnulib

On 12/24/20 7:16 AM, Adhemerval Zanella wrote:
> It sync with gnulib version d9c121346 with the following difference
> require fix a glibc build:

As I mentioned today on bug-gnulib I installed some changes into Gnulib 
which are related, and it'd probably be better to sync with the latest 
version. (Unfortunately I didn't know about your changes, so there was 
some overlapping work here.)

> +# define FUNC_REALPATH_WORKS 1

I don't see why this change is needed, as the only use of that macro is 
in "#if !FUNC_REALPATH_WORKS || defined _LIBC", which is unaffected by 
this change since _LIBC is defined.

> @@ -270,7 +270,7 @@
>                 buf[n] = '\0';
> 
>                 char *extra_buf = extra_buffer.data;
> -              idx_t end_idx;
> +              idx_t end_idx = 0;
>                 if (end_in_extra_buffer)
>                   end_idx = end - extra_buf;
>                 idx_t len = strlen (end);

This change isn't needed, since end_idx is used only when 
end_in_extra_buffer is true.

Was the latter change put in only to pacify older GCC versions? (I don't 
get a warning with GCC 10.2.) If so, we should do the initialization 
only for those older versions; or Gnulib has a GCC_LINT feature for this 
sort of thing.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] Import idx.h from gnulib
  2020-12-24 15:16 ` [PATCH 2/5] Import idx.h from gnulib Adhemerval Zanella via Libc-alpha
@ 2020-12-24 22:53   ` Paul Eggert
  2020-12-25 20:34   ` Florian Weimer
  2020-12-31 23:12   ` Joseph Myers
  2 siblings, 0 replies; 22+ messages in thread
From: Paul Eggert @ 2020-12-24 22:53 UTC (permalink / raw
  To: Adhemerval Zanella, libc-alpha; +Cc: bug-gnulib

Thanks, this looks good. Also, the following "[PATCH 3/5] Import 
filename.h from gnulib" looks good. I merged both changes into Gnulib here:

http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=1013adf6bf48627f84a9043cf2631ff13d2a452f

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]
  2020-12-24 15:17 ` [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970] Adhemerval Zanella via Libc-alpha
@ 2020-12-25  0:27   ` Paul Eggert
  2020-12-28 11:42     ` Adhemerval Zanella via Libc-alpha
  0 siblings, 1 reply; 22+ messages in thread
From: Paul Eggert @ 2020-12-25  0:27 UTC (permalink / raw
  To: Adhemerval Zanella; +Cc: libc-alpha, bug-gnulib

[-- 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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] warnings in canonicalize.c
  2020-12-24 22:45   ` Paul Eggert
@ 2020-12-25  0:44     ` Bruno Haible
  2020-12-25  5:56       ` Paul Eggert
  2020-12-28 12:53     ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
  1 sibling, 1 reply; 22+ messages in thread
From: Bruno Haible @ 2020-12-25  0:44 UTC (permalink / raw
  To: bug-gnulib; +Cc: libc-alpha

Paul Eggert wrote:
> > @@ -270,7 +270,7 @@
> >                 buf[n] = '\0';
> > 
> >                 char *extra_buf = extra_buffer.data;
> > -              idx_t end_idx;
> > +              idx_t end_idx = 0;
> >                 if (end_in_extra_buffer)
> >                   end_idx = end - extra_buf;
> >                 idx_t len = strlen (end);
> 
> This change isn't needed, since end_idx is used only when 
> end_in_extra_buffer is true.
> 
> Was the latter change put in only to pacify older GCC versions?

Yes.

> (I don't get a warning with GCC 10.2.)

Indeed, I don't see warnings in canonicalize-lgpl.c. But there are warnings
in gnulib's canonicalize.c:

gcc 9.3.0:

$ gcc -DHAVE_CONFIG_H -I. -I.. -Wall -g -O2 -c ./canonicalize.c                                    
./canonicalize.c: In function 'canonicalize_filename_mode_stk':
./canonicalize.c:337:16: warning: unused variable 'discard' [-Wunused-variable]
  337 |           char discard;
      |                ^~~~~~~
./canonicalize.c: In function 'canonicalize_filename_mode':
./canonicalize.c:400:33: warning: 'end_idx' may be used uninitialized in this function [-Wmaybe-uninitialized]
  400 |                 end = extra_buf + end_idx;
      |                       ~~~~~~~~~~^~~~~~~~~
./canonicalize.c:389:21: note: 'end_idx' was declared here
  389 |               idx_t end_idx;
      |                     ^~~~~~~

gcc 10.2.0:

$ gcc -DHAVE_CONFIG_H -I. -I.. -Wall -g -O2 -c ./canonicalize.c                                    
./canonicalize.c: In function 'canonicalize_filename_mode_stk':
./canonicalize.c:337:16: warning: unused variable 'discard' [-Wunused-variable]
  337 |           char discard;
      |                ^~~~~~~
...
./canonicalize.c:400:33: warning: 'end_idx' may be used uninitialized in this function [-Wmaybe-uninitialized]
  400 |                 end = extra_buf + end_idx;
      |                       ~~~~~~~~~~^~~~~~~~~
./canonicalize.c:389:21: note: 'end_idx' was declared here
  389 |               idx_t end_idx;
      |                     ^~~~~~~

Bruno


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] warnings in canonicalize.c
  2020-12-25  0:44     ` [PATCH 1/5] warnings in canonicalize.c Bruno Haible
@ 2020-12-25  5:56       ` Paul Eggert
  0 siblings, 0 replies; 22+ messages in thread
From: Paul Eggert @ 2020-12-25  5:56 UTC (permalink / raw
  To: Bruno Haible; +Cc: bug-gnulib, libc-alpha

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

On 12/24/20 4:44 PM, Bruno Haible wrote:
> Indeed, I don't see warnings in canonicalize-lgpl.c. But there are warnings
> in gnulib's canonicalize.c:

Thanks, I fixed those, along one or two others I noticed when compiling 
with GCC (Ubuntu 10.2.0-13ubuntu1). Even though I didn't get these 
warnings with canonicalize-lgpl.c I guess they're likely to occur and 
anyway it's better to keep the two files in sync when possible. So I 
installed the attached patch, which should fix the issues you noticed 
and should also propagate the warnings patches into canonicalize-lgpl.c. 
I hope this fixes the bogus warnings that Adhemerval noticed when 
building glibc's stdlib/canonicalize.c.

[-- Attachment #2: 0001-canonicalize-canonicalize-lgpl-remove-lint.patch --]
[-- Type: text/x-patch, Size: 5236 bytes --]

From abc2b3fe5af97e971f10ebdbeee174f6d87f1fe6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 24 Dec 2020 21:33:18 -0800
Subject: [PATCH] canonicalize, canonicalize-lgpl: remove lint

Pacify GCC.  Some of these problems were reported by Bruno Haible in:
https://lists.gnu.org/r/bug-gnulib/2020-12/msg00217.html
* lib/canonicalize-lgpl.c, lib/canonicalize.c:
Sort shared include directives, for consistency.
(IF_LINT): New macro.
(suffix_requires_dir_check): Mark with _GL_ATTRIBUTE_PURE.
* lib/canonicalize-lgpl.c (GCC_LINT, _GL_ATTRIBUTE_PURE) [_LIBC]:
New macros.
(realpath_stk): Suppress bogus -Wmaybe-uninitialized warning.
* lib/canonicalize.c (canonicalize_filename_mode_stk):
Omit unused local.  Suppress bogus -Wmaybe-uninitialized warning.
---
 ChangeLog               | 15 +++++++++++++++
 lib/canonicalize-lgpl.c | 15 ++++++++++++---
 lib/canonicalize.c      | 16 +++++++++++-----
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8f27b286e..dff97afe4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2020-12-24  Paul Eggert  <eggert@cs.ucla.edu>
+
+	canonicalize, canonicalize-lgpl: remove lint
+	Pacify GCC.  Some of these problems were reported by Bruno Haible in:
+	https://lists.gnu.org/r/bug-gnulib/2020-12/msg00217.html
+	* lib/canonicalize-lgpl.c, lib/canonicalize.c:
+	Sort shared include directives, for consistency.
+	(IF_LINT): New macro.
+	(suffix_requires_dir_check): Mark with _GL_ATTRIBUTE_PURE.
+	* lib/canonicalize-lgpl.c (GCC_LINT, _GL_ATTRIBUTE_PURE) [_LIBC]:
+	New macros.
+	(realpath_stk): Suppress bogus -Wmaybe-uninitialized warning.
+	* lib/canonicalize.c (canonicalize_filename_mode_stk):
+	Omit unused local.  Suppress bogus -Wmaybe-uninitialized warning.
+
 2020-12-24  Bruno Haible  <bruno@clisp.org>
 
 	spawn-pipe: Use posix_spawn by default on native Windows.
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 8c8f98abc..68b6fdf6e 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -38,8 +38,8 @@
 #include <unistd.h>
 
 #include <eloop-threshold.h>
-#include <idx.h>
 #include <filename.h>
+#include <idx.h>
 #include <scratch_buffer.h>
 
 #ifdef _LIBC
@@ -50,6 +50,8 @@
 # else
 #  define FACCESSAT_NEVER_EOVERFLOWS true
 # endif
+# define GCC_LINT 1
+# define _GL_ATTRIBUTE_PURE __attribute__ ((__pure__))
 #else
 # define __canonicalize_file_name canonicalize_file_name
 # define __realpath realpath
@@ -82,6 +84,13 @@
 # define __stat stat
 #endif
 
+/* Suppress bogus GCC -Wmaybe-uninitialized warnings.  */
+#if defined GCC_LINT || defined lint
+# define IF_LINT(Code) Code
+#else
+# define IF_LINT(Code) /* empty */
+#endif
+
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
 #endif
@@ -114,7 +123,7 @@ file_accessible (char const *file)
    component within END.  END must either be empty, or start with a
    slash.  */
 
-static bool
+static bool _GL_ATTRIBUTE_PURE
 suffix_requires_dir_check (char const *end)
 {
   /* If END does not start with a slash, the suffix is OK.  */
@@ -338,7 +347,7 @@ realpath_stk (const char *name, char *resolved,
               buf[n] = '\0';
 
               char *extra_buf = extra_buffer.data;
-              idx_t end_idx;
+              idx_t end_idx IF_LINT (= 0);
               if (end_in_extra_buffer)
                 end_idx = end - extra_buf;
               idx_t len = strlen (end);
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index 2c88335bf..e8b74d855 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -27,14 +27,21 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include <filename.h>
+#include <idx.h>
 #include <scratch_buffer.h>
 
 #include "attribute.h"
 #include "file-set.h"
-#include "idx.h"
 #include "hash-triple.h"
 #include "xalloc.h"
-#include "filename.h"
+
+/* Suppress bogus GCC -Wmaybe-uninitialized warnings.  */
+#if defined GCC_LINT || defined lint
+# define IF_LINT(Code) Code
+#else
+# define IF_LINT(Code) /* empty */
+#endif
 
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
@@ -72,7 +79,7 @@ file_accessible (char const *file)
    component within END.  END must either be empty, or start with a
    slash.  */
 
-static bool
+static bool _GL_ATTRIBUTE_PURE
 suffix_requires_dir_check (char const *end)
 {
   /* If END does not start with a slash, the suffix is OK.  */
@@ -334,7 +341,6 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
           dest = mempcpy (dest, start, startlen);
           *dest = '\0';
 
-          char discard;
           char *buf;
           ssize_t n = -1;
           if (!logical)
@@ -386,7 +392,7 @@ canonicalize_filename_mode_stk (const char *name, canonicalize_mode_t can_mode,
               buf[n] = '\0';
 
               char *extra_buf = extra_buffer.data;
-              idx_t end_idx;
+              idx_t end_idx IF_LINT (= 0);
               if (end_in_extra_buffer)
                 end_idx = end - extra_buf;
               idx_t len = strlen (end);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] Import idx.h from gnulib
  2020-12-24 15:16 ` [PATCH 2/5] Import idx.h from gnulib Adhemerval Zanella via Libc-alpha
  2020-12-24 22:53   ` Paul Eggert
@ 2020-12-25 20:34   ` Florian Weimer
  2020-12-25 21:38     ` Bruno Haible
  2020-12-26  1:29     ` Paul Eggert
  2020-12-31 23:12   ` Joseph Myers
  2 siblings, 2 replies; 22+ messages in thread
From: Florian Weimer @ 2020-12-25 20:34 UTC (permalink / raw
  To: Adhemerval Zanella via Libc-alpha; +Cc: bug-gnulib

* Adhemerval Zanella via Libc-alpha:

> diff --git a/include/idx.h b/include/idx.h
> new file mode 100644
> index 0000000000..ad7d31a2bc
> --- /dev/null
> +++ b/include/idx.h
> @@ -0,0 +1,113 @@
> +/* A type for indices and sizes.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   This program 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 General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, see <https://www.gnu.org/licenses/>.  */

This file is linked into the library itself, so we can only accept
something that is under a compatible license (probably LGPLv2 or later).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] Import idx.h from gnulib
  2020-12-25 20:34   ` Florian Weimer
@ 2020-12-25 21:38     ` Bruno Haible
  2020-12-26  1:29     ` Paul Eggert
  1 sibling, 0 replies; 22+ messages in thread
From: Bruno Haible @ 2020-12-25 21:38 UTC (permalink / raw
  To: bug-gnulib; +Cc: Florian Weimer, libc-alpha

Florian Weimer wrote:
> > diff --git a/include/idx.h b/include/idx.h
> > new file mode 100644
> > index 0000000000..ad7d31a2bc
> > --- /dev/null
> > +++ b/include/idx.h
> > @@ -0,0 +1,113 @@
> > +/* A type for indices and sizes.
> > +
> > +   Copyright (C) 2020 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 2, or (at your option)
> > +   any later version.
> > +
> > +   This program 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 General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program; if not, see <https://www.gnu.org/licenses/>.  */
> 
> This file is linked into the library itself, so we can only accept
> something that is under a compatible license (probably LGPLv2 or later).

The file is actually under LGPLv2+ [1][2]. You can replace the license
header with an LGPLv2+ header.

Bruno

[1] https://www.gnu.org/software/gnulib/manual/html_node/Gnulib-licensing.html
[2] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=modules/idx


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] Import idx.h from gnulib
  2020-12-25 20:34   ` Florian Weimer
  2020-12-25 21:38     ` Bruno Haible
@ 2020-12-26  1:29     ` Paul Eggert
  1 sibling, 0 replies; 22+ messages in thread
From: Paul Eggert @ 2020-12-26  1:29 UTC (permalink / raw
  To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha, bug-gnulib

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

On 12/25/20 12:34 PM, Florian Weimer wrote:
> This file is linked into the library itself, so we can only accept
> something that is under a compatible license (probably LGPLv2 or later).

I installed the attached patch to the Gnulib copy of idx.h to do that. 
This uses the same wording as what's in glibc's scratch_buffer.h.

[-- Attachment #2: 0001-idx-change-idx.h-comment-to-LGPLv2.1.patch --]
[-- Type: text/x-patch, Size: 2545 bytes --]

From e0ea25e27e1f1ed70b5dcc2338b9ede365f9a79a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 25 Dec 2020 17:26:51 -0800
Subject: [PATCH] idx: change idx.h comment to LGPLv2.1+

* lib/idx.h: Change license notice to match what should be in glibc.
gnulib-tool will change it as appropriate anyway, so this is just
to simplify syncing with glibc.
---
 ChangeLog |  7 +++++++
 lib/idx.h | 21 +++++++++++----------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 16112f73e..82362718d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2020-12-25  Paul Eggert  <eggert@cs.ucla.edu>
+
+	idx: change idx.h comment to LGPLv2.1+
+	* lib/idx.h: Change license notice to match what should be in glibc.
+	gnulib-tool will change it as appropriate anyway, so this is just
+	to simplify syncing with glibc.
+
 2020-12-25  Thien-Thi Nguyen  <ttn@gnu.org>
 
 	MODULES.html.sh: Update after 2020-12-19 change.
diff --git a/lib/idx.h b/lib/idx.h
index ad7d31a2b..024b44ae9 100644
--- a/lib/idx.h
+++ b/lib/idx.h
@@ -1,19 +1,20 @@
 /* A type for indices and sizes.
-
    Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
 
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 2, or (at your option)
-   any later version.
+   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.
 
-   This program is distributed in the hope that it will be useful,
+   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 General Public License for more details.
+   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 General Public License
-   along with this program; if not, see <https://www.gnu.org/licenses/>.  */
+   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 _IDX_H
 #define _IDX_H
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]
  2020-12-25  0:27   ` Paul Eggert
@ 2020-12-28 11:42     ` Adhemerval Zanella via Libc-alpha
  2020-12-28 13:32       ` Adhemerval Zanella via Libc-alpha
  2020-12-28 21:04       ` Paul Eggert
  0 siblings, 2 replies; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-28 11:42 UTC (permalink / raw
  To: Paul Eggert; +Cc: libc-alpha, bug-gnulib



On 24/12/2020 21:27, Paul Eggert wrote:
> 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.)

Indeed this seems a better alternative.

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

Which changes are you referring? Checking against bbaba6ce5 I see all
glibc files for scratch_buffer are similar to the gnulib ones.

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

Yes, this trades a stat call by an extra readlink call.  However it seems
that your strategy to use faccessat should be better, assuming it is lighter
syscall.

> 
>> -  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.

I wasn't aware on the GCC issue in fact (it seems to affect GCC 10/11
and I am using GCC 9.2.1).

> 
>> @@ -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.

Right.

> 
>> +          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.
> 

Indeed, this is a left over from development.

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

Yes, I noted now on gnulib master that you handled it with an faccessat 
on dir_check. 

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

Yes, I think this is also a leftover from development.

> 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.

Changes seems ok, I will send a v3 with the proposed changes to sync
the gnulib headers, along with the scratch_buffer change, the gnulib
sync that fix all the issues, and the extra test.

Thanks for working on this.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241]
  2020-12-24 22:45   ` Paul Eggert
  2020-12-25  0:44     ` [PATCH 1/5] warnings in canonicalize.c Bruno Haible
@ 2020-12-28 12:53     ` Adhemerval Zanella via Libc-alpha
  1 sibling, 0 replies; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-28 12:53 UTC (permalink / raw
  To: Paul Eggert; +Cc: libc-alpha, bug-gnulib



On 24/12/2020 19:45, Paul Eggert wrote:
> On 12/24/20 7:16 AM, Adhemerval Zanella wrote:
>> It sync with gnulib version d9c121346 with the following difference
>> require fix a glibc build:
> 
> As I mentioned today on bug-gnulib I installed some changes into Gnulib which are related, and it'd probably be better to sync with the latest version. (Unfortunately I didn't know about your changes, so there was some overlapping work here.)
> 
>> +# define FUNC_REALPATH_WORKS 1
> 
> I don't see why this change is needed, as the only use of that macro is in "#if !FUNC_REALPATH_WORKS || defined _LIBC", which is unaffected by this change since _LIBC is defined.

I am seeing this with gcc 9.2.1:

canonicalize.c:101:6: error: "FUNC_REALPATH_WORKS" is not defined, evaluates to 0 [-Werror=undef]
  101 | #if !FUNC_REALPATH_WORKS || defined _LIBC

(We build glibc with -Wundef -Werror).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]
  2020-12-28 11:42     ` Adhemerval Zanella via Libc-alpha
@ 2020-12-28 13:32       ` Adhemerval Zanella via Libc-alpha
  2020-12-28 21:04       ` Paul Eggert
  1 sibling, 0 replies; 22+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-12-28 13:32 UTC (permalink / raw
  To: Paul Eggert; +Cc: libc-alpha, bug-gnulib



On 28/12/2020 08:42, Adhemerval Zanella wrote:
>>>   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).
> 
> Yes, this trades a stat call by an extra readlink call.  However it seems
> that your strategy to use faccessat should be better, assuming it is lighter
> syscall.

Also, it would require a recent kernel (5.8) to avoid faccessat to fallback
to use __NR_faccessat (it would make each call to faccessat2 to issue
two syscall).  We can live with this and if it turns a performance issue we
ca the hack to set a global variable to avoid it.

Now this snippet:

 47 # include <sysdep.h>
 48 # ifdef __ASSUME_FACCESSAT2
 49 #  define FACCESSAT_NEVER_EOVERFLOWS __ASSUME_FACCESSAT2
 50 # else
 51 #  define FACCESSAT_NEVER_EOVERFLOWS true
 52 # endif

is not required, since Linux faccessat fallback uses LFS stat call that
does not return EOVERFLOW.  And it also bleeds Linux implementation details
on generic code.

I will send a patch to fix it.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]
  2020-12-28 11:42     ` Adhemerval Zanella via Libc-alpha
  2020-12-28 13:32       ` Adhemerval Zanella via Libc-alpha
@ 2020-12-28 21:04       ` Paul Eggert
  1 sibling, 0 replies; 22+ messages in thread
From: Paul Eggert @ 2020-12-28 21:04 UTC (permalink / raw
  To: Adhemerval Zanella; +Cc: libc-alpha, bug-gnulib

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

On 12/28/20 3:42 AM, Adhemerval Zanella wrote:

> On 24/12/2020 21:27, Paul Eggert wrote:

>> Also, shouldn't we merge the already-existing Gnulib scratch_buffer changes into glibc, along with this new change?
> 
> Which changes are you referring? Checking against bbaba6ce5 I see all
> glibc files for scratch_buffer are similar to the gnulib ones.

Sorry false alarm. Don't know what I was thinking of.

I incorporated almost all the changes suggested by your 
recently-proposed glibc changes into Gnulib, by installing the attached 
into Gnulib savannah master. There are still a few minor discrepancies 
from what you proposed for glibc, and I plan to comment on those 
discrepancies in reply to your other emails.

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

From 62c8b7b665b7c4c54d53496407342c5aefc9f8d1 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 28 Dec 2020 11:38:58 -0800
Subject: [PATCH 1/3] 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                           | 12 +++++++++
 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, 83 insertions(+), 22 deletions(-)
 create mode 100644 lib/malloc/scratch_buffer_dupfree.c

diff --git a/ChangeLog b/ChangeLog
index 97966b427..0428619c1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2020-12-28  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.
+
 2020-12-27  Paul Eggert  <eggert@cs.ucla.edu>
 
 	regex: remove glibc21.m4
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 68b6fdf6e..7ac5d412d 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -413,24 +413,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 e8b74d855..48a49e412 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -469,11 +469,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


[-- Attachment #3: 0002-canonicalize-lgpl-accommodate-picky-cpp.patch --]
[-- Type: text/x-patch, Size: 1573 bytes --]

From 7796b1f6235a0328dc411d51d0da45ddda6d575d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 28 Dec 2020 11:58:38 -0800
Subject: [PATCH 2/3] canonicalize-lgpl: accommodate picky cpp

* lib/canonicalize-lgpl.c: Use "defined FUNC_REALPATH_WORKS" in
case preprocessor is picky.  Reported by Adhemerval Zanella in:
https://sourceware.org/pipermail/libc-alpha/2020-December/121130.html
---
 ChangeLog               | 5 +++++
 lib/canonicalize-lgpl.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 0428619c1..9a91eda92 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2020-12-28  Paul Eggert  <eggert@cs.ucla.edu>
 
+	canonicalize-lgpl: accommodate picky cpp
+	* lib/canonicalize-lgpl.c: Use "defined FUNC_REALPATH_WORKS" in
+	case preprocessor is picky.  Reported by Adhemerval Zanella in:
+	https://sourceware.org/pipermail/libc-alpha/2020-December/121130.html
+
 	canonicalize: simplify via scratch_buffer_dupfree
 	* config/srclist.txt: Adjust accordingly.
 	* lib/canonicalize-lgpl.c (realpath_stk):
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 7ac5d412d..332b5bab4 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -98,7 +98,7 @@
 # define FACCESSAT_NEVER_EOVERFLOWS false
 #endif
 
-#if !FUNC_REALPATH_WORKS || defined _LIBC
+#if defined _LIBC || !FUNC_REALPATH_WORKS
 
 /* Return true if FILE's existence can be shown, false (setting errno)
    otherwise.  Follow symbolic links.  */
-- 
2.27.0


[-- Attachment #4: 0003-faccessat-revert-recent-EOVERFLOW-change.patch --]
[-- Type: text/x-patch, Size: 10863 bytes --]

From 20527b26f5e020ebaa48a8d5daec4cccebf051df Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 28 Dec 2020 12:38:52 -0800
Subject: [PATCH 3/3] faccessat: revert recent EOVERFLOW change
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

I misunderstood the glibc source code.  Deduced from
Adhemerval Zanella’s proposed glibc patch in:
https://sourceware.org/pipermail/libc-alpha/2020-December/121131.html
* doc/posix-functions/faccessat.texi: It is not a problem.
* lib/canonicalize-lgpl.c, lib/canonicalize.c, lib/faccessat.c:
(FACCESSAT_NEVER_OVERFLOWS): Remove. All uses removed.
* lib/faccessat.c: Revert to simpler version now that
LSTAT_FOLLOWS_SLASHED_SYMLINK must be false.
* m4/faccessat.m4 (gl_FUNC_FACCESSAT_EOVERFLOW):
Remove.  All uses removed.
* modules/canonicalize, modules/canonicalize-lgpl (Files):
Remove m4/faccessat.m4.
---
 ChangeLog                          | 14 +++++++++++++
 doc/posix-functions/faccessat.texi | 14 +++----------
 lib/canonicalize-lgpl.c            | 16 ++-------------
 lib/canonicalize.c                 | 10 ++-------
 lib/faccessat.c                    | 14 +------------
 m4/canonicalize.m4                 |  4 +---
 m4/faccessat.m4                    | 33 +++---------------------------
 modules/canonicalize               |  1 -
 modules/canonicalize-lgpl          |  1 -
 9 files changed, 26 insertions(+), 81 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 9a91eda92..1481becc9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2020-12-28  Paul Eggert  <eggert@cs.ucla.edu>
 
+	faccessat: revert recent EOVERFLOW change
+	I misunderstood the glibc source code.  Deduced from
+	Adhemerval Zanella’s proposed glibc patch in:
+	https://sourceware.org/pipermail/libc-alpha/2020-December/121131.html
+	* doc/posix-functions/faccessat.texi: It is not a problem.
+	* lib/canonicalize-lgpl.c, lib/canonicalize.c, lib/faccessat.c:
+	(FACCESSAT_NEVER_OVERFLOWS): Remove. All uses removed.
+	* lib/faccessat.c: Revert to simpler version now that
+	LSTAT_FOLLOWS_SLASHED_SYMLINK must be false.
+	* m4/faccessat.m4 (gl_FUNC_FACCESSAT_EOVERFLOW):
+	Remove.  All uses removed.
+	* modules/canonicalize, modules/canonicalize-lgpl (Files):
+	Remove m4/faccessat.m4.
+
 	canonicalize-lgpl: accommodate picky cpp
 	* lib/canonicalize-lgpl.c: Use "defined FUNC_REALPATH_WORKS" in
 	case preprocessor is picky.  Reported by Adhemerval Zanella in:
diff --git a/doc/posix-functions/faccessat.texi b/doc/posix-functions/faccessat.texi
index 07ea8e7bf..5d5165e47 100644
--- a/doc/posix-functions/faccessat.texi
+++ b/doc/posix-functions/faccessat.texi
@@ -15,12 +15,6 @@ glibc 2.3.6, macOS 10.12, FreeBSD 7.4, NetBSD 6.1.5, OpenBSD 4.9, Minix 3.1.8, A
 On some platforms, @code{faccessat (dfd, "file/", amode, flag)}
 succeeds instead of failing when @file{file} is not a directory.
 macOS 10.13.
-@item
-On some platforms, @code{faccessat} can incorrectly fail with
-@code{EOVERFLOW} when the mode is @code{F_OK}:
-GNU/Linux with glibc 2.32, or with Linux kernel 5.7.
-@c This bug should be fixed in glibc 2.33 and kernel 5.8.  See:
-@c https://sourceware.org/bugzilla/show_bug.cgi?id=18683
 @end itemize
 
 Portability problems not fixed by Gnulib:
@@ -36,11 +30,9 @@ The replacement does not support the @code{AT_SYMLINK_NOFOLLOW} flag,
 which is supported by GNU @code{faccessat}.
 @item
 On some platforms, @code{faccessat} can mishandle @code{AT_EACCESS}
-after a process starts as root and then becomes non-root,
-or can incorrectly fail with @code{EOVERFLOW} when the mode
-is not @code{F_OK}:
-GNU/Linux with glibc 2.32, or with Linux kernel 5.7.
-@c These bugs should be fixed in glibc 2.33 and kernel 5.8.  See:
+after a process starts as root and then becomes non-root:
+GNU/Linux with glibc 2.32.
+@c This bug should be fixed in glibc 2.33.  See:
 @c https://sourceware.org/bugzilla/show_bug.cgi?id=18683
 @end itemize
 
diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
index 332b5bab4..04fe95253 100644
--- a/lib/canonicalize-lgpl.c
+++ b/lib/canonicalize-lgpl.c
@@ -44,12 +44,6 @@
 
 #ifdef _LIBC
 # include <shlib-compat.h>
-# include <sysdep.h>
-# ifdef __ASSUME_FACCESSAT2
-#  define FACCESSAT_NEVER_EOVERFLOWS __ASSUME_FACCESSAT2
-# else
-#  define FACCESSAT_NEVER_EOVERFLOWS true
-# endif
 # define GCC_LINT 1
 # define _GL_ATTRIBUTE_PURE __attribute__ ((__pure__))
 #else
@@ -94,9 +88,6 @@
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
 #endif
-#ifndef FACCESSAT_NEVER_EOVERFLOWS
-# define FACCESSAT_NEVER_EOVERFLOWS false
-#endif
 
 #if defined _LIBC || !FUNC_REALPATH_WORKS
 
@@ -106,14 +97,11 @@ static bool
 file_accessible (char const *file)
 {
 # if defined _LIBC || HAVE_FACCESSAT
-  int r = __faccessat (AT_FDCWD, file, F_OK, AT_EACCESS);
+  return __faccessat (AT_FDCWD, file, F_OK, AT_EACCESS) == 0;
 # else
   struct stat st;
-  int r = __stat (file, &st);
+  return __stat (file, &st) == 0 || errno == EOVERFLOW;
 # endif
-
-  return ((!FACCESSAT_NEVER_EOVERFLOWS && r < 0 && errno == EOVERFLOW)
-          || r == 0);
 }
 
 /* True if concatenating END as a suffix to a file name means that the
diff --git a/lib/canonicalize.c b/lib/canonicalize.c
index 48a49e412..a4d3aab96 100644
--- a/lib/canonicalize.c
+++ b/lib/canonicalize.c
@@ -46,9 +46,6 @@
 #ifndef DOUBLE_SLASH_IS_DISTINCT_ROOT
 # define DOUBLE_SLASH_IS_DISTINCT_ROOT false
 #endif
-#ifndef FACCESSAT_NEVER_EOVERFLOWS
-# define FACCESSAT_NEVER_EOVERFLOWS false
-#endif
 
 #if ISSLASH ('\\')
 # define SLASHES "/\\"
@@ -62,14 +59,11 @@ static bool
 file_accessible (char const *file)
 {
 # if HAVE_FACCESSAT
-  int r = faccessat (AT_FDCWD, file, F_OK, AT_EACCESS);
+  return faccessat (AT_FDCWD, file, F_OK, AT_EACCESS) == 0;
 # else
   struct stat st;
-  int r = stat (file, &st);
+  return stat (file, &st) == 0 || errno == EOVERFLOW;
 # endif
-
-  return ((!FACCESSAT_NEVER_EOVERFLOWS && r < 0 && errno == EOVERFLOW)
-          || r == 0);
 }
 
 /* True if concatenating END as a suffix to a file name means that the
diff --git a/lib/faccessat.c b/lib/faccessat.c
index 330c54a0b..9f6a11bf6 100644
--- a/lib/faccessat.c
+++ b/lib/faccessat.c
@@ -32,13 +32,6 @@
 #include <sys/stat.h>
 #undef _GL_INCLUDING_UNISTD_H
 
-#ifndef FACCESSAT_NEVER_EOVERFLOWS
-# define FACCESSAT_NEVER_EOVERFLOWS 0
-#endif
-#ifndef LSTAT_FOLLOWS_SLASHED_SYMLINK
-# define LSTAT_FOLLOWS_SLASHED_SYMLINK 0
-#endif
-
 #if HAVE_FACCESSAT
 static int
 orig_faccessat (int fd, char const *name, int mode, int flag)
@@ -66,12 +59,7 @@ rpl_faccessat (int fd, char const *file, int mode, int flag)
 {
   int result = orig_faccessat (fd, file, mode, flag);
 
-  if (result != 0)
-    {
-      if (!FACCESSAT_NEVER_EOVERFLOWS && mode == F_OK && errno == EOVERFLOW)
-        return 0;
-    }
-  else if (!LSTAT_FOLLOWS_SLASHED_SYMLINK && file[strlen (file) - 1] == '/')
+  if (result == 0 && file[strlen (file) - 1] == '/')
     {
       struct stat st;
       result = fstatat (fd, file, &st, 0);
diff --git a/m4/canonicalize.m4 b/m4/canonicalize.m4
index c8da4dfcb..404db4cb6 100644
--- a/m4/canonicalize.m4
+++ b/m4/canonicalize.m4
@@ -1,4 +1,4 @@
-# canonicalize.m4 serial 34
+# canonicalize.m4 serial 35
 
 dnl Copyright (C) 2003-2007, 2009-2020 Free Software Foundation, Inc.
 
@@ -11,7 +11,6 @@ dnl with or without modifications, as long as this notice is preserved.
 AC_DEFUN([gl_FUNC_CANONICALIZE_FILENAME_MODE],
 [
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
-  AC_REQUIRE([gl_FUNC_FACCESSAT_EOVERFLOW])
   AC_REQUIRE([gl_FUNC_LSTAT_FOLLOWS_SLASHED_SYMLINK])
   AC_CHECK_FUNCS_ONCE([canonicalize_file_name faccessat])
   AC_REQUIRE([gl_DOUBLE_SLASH_ROOT])
@@ -58,7 +57,6 @@ AC_DEFUN([gl_CANONICALIZE_LGPL],
 AC_DEFUN([gl_CANONICALIZE_LGPL_SEPARATE],
 [
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
-  AC_REQUIRE([gl_FUNC_FACCESSAT_EOVERFLOW])
   AC_REQUIRE([gl_FUNC_LSTAT_FOLLOWS_SLASHED_SYMLINK])
   AC_CHECK_FUNCS_ONCE([canonicalize_file_name faccessat])
 
diff --git a/m4/faccessat.m4 b/m4/faccessat.m4
index a4ad31a46..b8da51e4c 100644
--- a/m4/faccessat.m4
+++ b/m4/faccessat.m4
@@ -1,4 +1,4 @@
-# serial 9
+# serial 10
 # See if we need to provide faccessat replacement.
 
 dnl Copyright (C) 2009-2020 Free Software Foundation, Inc.
@@ -8,31 +8,6 @@ dnl with or without modifications, as long as this notice is preserved.
 
 # Written by Eric Blake.
 
-AC_DEFUN([gl_FUNC_FACCESSAT_EOVERFLOW],
-[
-  AC_CHECK_FUNCS_ONCE([faccessat])
-  if test "$ac_cv_func_faccessat" = yes; then
-    AC_CACHE_CHECK([whether faccessat works when stat would EOVERFLOW],
-      [gl_cv_func_faccessat_never_eoverflows],
-      [AC_COMPILE_IFELSE(
-         [AC_LANG_PROGRAM([],
-            [[#ifdef __linux__
-               #include <linux/version.h>
-               #if (! (KERNEL_VERSION (5, 8, 0) <= LINUX_VERSION_CODE \
-                    && 2 < (__GLIBC__ + (33 <= __GLIBC_MINOR__))))
-                #error "faccessat might fail with EOVERFLOW"
-               #endif
-              #endif
-            ]])],
-         [gl_cv_func_faccessat_never_eoverflows=yes],
-         [gl_cv_func_faccessat_never_eoverflows=no])])
-    if test "$gl_cv_func_faccessat_never_eoverflows" = yes; then
-      AC_DEFINE([FACCESSAT_NEVER_EOVERFLOWS], 1,
-        [Define to 1 if faccessat is EOVERFLOW-free.])
-    fi
-  fi
-])
-
 AC_DEFUN([gl_FUNC_FACCESSAT],
 [
   AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
@@ -41,14 +16,12 @@ AC_DEFUN([gl_FUNC_FACCESSAT],
   dnl Persuade glibc <unistd.h> to declare faccessat().
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
 
-  AC_REQUIRE([gl_FUNC_FACCESSAT_EOVERFLOW])
-
   AC_CHECK_FUNCS_ONCE([faccessat])
   if test $ac_cv_func_faccessat = no; then
     HAVE_FACCESSAT=0
   else
-    case $gl_cv_func_lstat_dereferences_slashed_symlink,$gl_cv_func_faccessat_never_eoverflows in
-      *yes,*yes) ;;
+    case $gl_cv_func_lstat_dereferences_slashed_symlink in
+      *yes) ;;
       *)    REPLACE_FACCESSAT=1 ;;
     esac
   fi
diff --git a/modules/canonicalize b/modules/canonicalize
index 29919bcdc..5003f2682 100644
--- a/modules/canonicalize
+++ b/modules/canonicalize
@@ -5,7 +5,6 @@ Files:
 lib/canonicalize.h
 lib/canonicalize.c
 m4/canonicalize.m4
-m4/faccessat.m4
 m4/lstat.m4
 
 Depends-on:
diff --git a/modules/canonicalize-lgpl b/modules/canonicalize-lgpl
index b5f3e7f69..a96f9011e 100644
--- a/modules/canonicalize-lgpl
+++ b/modules/canonicalize-lgpl
@@ -5,7 +5,6 @@ Files:
 lib/canonicalize-lgpl.c
 m4/canonicalize.m4
 m4/double-slash-root.m4
-m4/faccessat.m4
 m4/lstat.m4
 
 Depends-on:
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] Import idx.h from gnulib
  2020-12-24 15:16 ` [PATCH 2/5] Import idx.h from gnulib Adhemerval Zanella via Libc-alpha
  2020-12-24 22:53   ` Paul Eggert
  2020-12-25 20:34   ` Florian Weimer
@ 2020-12-31 23:12   ` Joseph Myers
  2021-01-01  3:24     ` Paul Eggert
  2 siblings, 1 reply; 22+ messages in thread
From: Joseph Myers @ 2020-12-31 23:12 UTC (permalink / raw
  To: Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha

On Thu, 24 Dec 2020, Adhemerval Zanella via Libc-alpha wrote:

> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.

Anything used in glibc libraries should be LGPLv2.1+, not GPL.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] Import filename.h from gnulib
  2020-12-24 15:16 ` [PATCH 3/5] Import filename.h " Adhemerval Zanella via Libc-alpha
@ 2020-12-31 23:13   ` Joseph Myers
  2021-01-01  3:31     ` Paul Eggert
  0 siblings, 1 reply; 22+ messages in thread
From: Joseph Myers @ 2020-12-31 23:13 UTC (permalink / raw
  To: Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha

On Thu, 24 Dec 2020, Adhemerval Zanella via Libc-alpha wrote:

> diff --git a/include/filename.h b/include/filename.h
> new file mode 100644
> index 0000000000..4598fb1d63
> --- /dev/null
> +++ b/include/filename.h
> @@ -0,0 +1,110 @@
> +/* Basic filename support macros.
> +   Copyright (C) 2001-2004, 2007-2020 Free Software Foundation, Inc.
> +
> +   This program is free software: you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.

Anything used in glibc libraries should be LGPLv2.1+.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] Import idx.h from gnulib
  2020-12-31 23:12   ` Joseph Myers
@ 2021-01-01  3:24     ` Paul Eggert
  0 siblings, 0 replies; 22+ messages in thread
From: Paul Eggert @ 2021-01-01  3:24 UTC (permalink / raw
  To: Joseph Myers, Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha

On 12/31/20 3:12 PM, Joseph Myers wrote:
> Anything used in glibc libraries should be LGPLv2.1+, not GPL.

That was fixed in the most recently-published version of the patch:

https://sourceware.org/pipermail/libc-alpha/2020-December/121178.html

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] Import filename.h from gnulib
  2020-12-31 23:13   ` Joseph Myers
@ 2021-01-01  3:31     ` Paul Eggert
  0 siblings, 0 replies; 22+ messages in thread
From: Paul Eggert @ 2021-01-01  3:31 UTC (permalink / raw
  To: Joseph Myers, Adhemerval Zanella; +Cc: bug-gnulib, libc-alpha

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

On 12/31/20 3:13 PM, Joseph Myers wrote:

> Anything used in glibc libraries should be LGPLv2.1+.

Thanks for catching that. I installed the attached patch into Gnulib so 
that glibc and Gnulib copies of filename.h can be identical.

[-- Attachment #2: 0001-filename-change-filename.h-comment-to-LGPLv2.1.patch --]
[-- Type: text/x-patch, Size: 2662 bytes --]

From ec51ca6c9d59037df66afe0804394f66a2a8ff54 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 31 Dec 2020 19:29:08 -0800
Subject: [PATCH] filename: change filename.h comment to LGPLv2.1+

* lib/filename.h: Change license notice to match what should be in
glibc, and what modules/filename specifies.  This is to simplify
syncing with glibc.
---
 ChangeLog      |  5 +++++
 lib/filename.h | 20 +++++++++++---------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e8b956828..f535c7805 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2020-12-31  Paul Eggert  <eggert@cs.ucla.edu>
 
+	filename: change filename.h comment to LGPLv2.1+
+	* lib/filename.h: Change license notice to match what should be in
+	glibc, and what modules/filename specifies.  This is to simplify
+	syncing with glibc.
+
 	doc: mention year-0 bugs in Solaris etc.
 	* doc/posix-functions/gmtime.texi, doc/posix-functions/gmtime_r.texi:
 	* doc/posix-functions/localtime.texi:
diff --git a/lib/filename.h b/lib/filename.h
index 4598fb1d6..984154c49 100644
--- a/lib/filename.h
+++ b/lib/filename.h
@@ -1,18 +1,20 @@
 /* Basic filename support macros.
    Copyright (C) 2001-2004, 2007-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
 
-   This program is free software: you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
+   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.
 
-   This program is distributed in the hope that it will be useful,
+   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 General Public License for more details.
+   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 General Public License
-   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+   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/>.  */
 
 /* From Paul Eggert and Jim Meyering.  */
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-01-01  3:31 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-24 15:16 [PATCH 0/5] Fix multiple realpath issues Adhemerval Zanella via Libc-alpha
2020-12-24 15:16 ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
2020-12-24 22:45   ` Paul Eggert
2020-12-25  0:44     ` [PATCH 1/5] warnings in canonicalize.c Bruno Haible
2020-12-25  5:56       ` Paul Eggert
2020-12-28 12:53     ` [PATCH 1/5] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26241] Adhemerval Zanella via Libc-alpha
2020-12-24 15:16 ` [PATCH 2/5] Import idx.h from gnulib Adhemerval Zanella via Libc-alpha
2020-12-24 22:53   ` Paul Eggert
2020-12-25 20:34   ` Florian Weimer
2020-12-25 21:38     ` Bruno Haible
2020-12-26  1:29     ` Paul Eggert
2020-12-31 23:12   ` Joseph Myers
2021-01-01  3:24     ` Paul Eggert
2020-12-24 15:16 ` [PATCH 3/5] Import filename.h " Adhemerval Zanella via Libc-alpha
2020-12-31 23:13   ` Joseph Myers
2021-01-01  3:31     ` Paul Eggert
2020-12-24 15:17 ` [PATCH 4/5] stdlib: Add testcase fro BZ #26241 Adhemerval Zanella via Libc-alpha
2020-12-24 15:17 ` [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970] Adhemerval Zanella via Libc-alpha
2020-12-25  0:27   ` Paul Eggert
2020-12-28 11:42     ` Adhemerval Zanella via Libc-alpha
2020-12-28 13:32       ` Adhemerval Zanella via Libc-alpha
2020-12-28 21:04       ` Paul Eggert

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