Re: [PATCH 1/6] canonicalize-lgpl: fix EOVERFLOW bug

[Adhemerval Zanella]

On 02/12/2020 19:39, Paul Eggert wrote:
> * lib/canonicalize-lgpl.c: Do not include <sys/stat.h>.
> (__realpath): Do not use lstat.  Just use readlink, as this
> suffices and it avoids the EOVERFLOW problem that lstat has.
> * modules/canonicalize-lgpl (Depends-on): Remove lstat, sys_stat.

I have sent a similar fix to reviews for this very issue for glibc 
(which is based on the canonicalize-lgpl) along with other fixes that
you might take a look at [1].

As I hinted on glibc BZ#24970 [2], your fix does not really address all
glibc regression tests.  It still fails with same 2 tests I described in
comment 2.

I think if we could remove the alloca usage I might try to sync with
gnulib version glibc code (I really want to fix BZ#26341 and also remove 
all possible alloca usage within glibc).

[1] https://patchwork.sourceware.org/project/glibc/list/?series=1062
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=24970
[3] https://sourceware.org/bugzilla/show_bug.cgi?id=26341

> ---
>  ChangeLog                 |  8 +++++++
>  lib/canonicalize-lgpl.c   | 49 ++++++++++++++-------------------------
>  modules/canonicalize-lgpl |  2 --
>  3 files changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index cbe4558d5..27afc2b4a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2020-12-02  Paul Eggert  <eggert@cs.ucla.edu>
> +
> +     canonicalize-lgpl: fix EOVERFLOW bug
> +     * lib/canonicalize-lgpl.c: Do not include <sys/stat.h>.
> +     (__realpath): Do not use lstat.  Just use readlink, as this
> +     suffices and it avoids the EOVERFLOW problem that lstat has.
> +     * modules/canonicalize-lgpl (Depends-on): Remove lstat, sys_stat.
> +
>  2020-12-02  Bruno Haible  <bruno@clisp.org>
>  
>       strsignal-tests: Fix test failure on macOS 10.13.
> diff --git a/lib/canonicalize-lgpl.c b/lib/canonicalize-lgpl.c
> index edac98f83..981300aa7 100644
> --- a/lib/canonicalize-lgpl.c
> +++ b/lib/canonicalize-lgpl.c
> @@ -36,7 +36,6 @@
>  #if HAVE_SYS_PARAM_H || defined _LIBC
>  # include <sys/param.h>
>  #endif
> -#include <sys/stat.h>
>  #include <errno.h>
>  #include <stddef.h>
>  
> @@ -198,12 +197,6 @@ __realpath (const char *name, char *resolved)
>  
>    for (end = start; *start; start = end)
>      {
> -#ifdef _LIBC
> -      struct stat64 st;
> -#else
> -      struct stat st;
> -#endif
> -
>        /* Skip sequence of multiple path-separators.  */
>        while (ISSLASH (*start))
>          ++start;
> @@ -212,9 +205,7 @@ __realpath (const char *name, char *resolved)
>        for (end = start; *end && !ISSLASH (*end); ++end)
>          /* Nothing.  */;
>  
> -      if (end - start == 0)
> -        break;
> -      else if (end - start == 1 && start[0] == '.')
> +      if (end - start == 1 && start[0] == '.')
>          /* nothing */;
>        else if (end - start == 2 && start[0] == '.' && start[1] == '.')
>          {
> @@ -272,20 +263,17 @@ __realpath (const char *name, char *resolved)
>  #endif
>            *dest = '\0';
>  
> -          /* FIXME: if lstat fails with errno == EOVERFLOW,
> -             the entry exists.  */
> -#ifdef _LIBC
> -          if (__lxstat64 (_STAT_VER, rpath, &st) < 0)
> -#else
> -          if (lstat (rpath, &st) < 0)
> -#endif
> -            goto error;
> -
> -          if (S_ISLNK (st.st_mode))
> +          char linkbuf[128];
> +          ssize_t n = __readlink (rpath, linkbuf, sizeof linkbuf);
> +          if (n < 0)
> +            {
> +              if (errno != EINVAL)
> +                goto error;
> +            }
> +          else
>              {
>                char *buf;
>                size_t len;
> -              ssize_t n;
>  
>                if (++num_links > MAXSYMLINKS)
>                  {
> @@ -302,11 +290,15 @@ __realpath (const char *name, char *resolved)
>                        goto error;
>                      }
>                  }
> -              buf = extra_buf + path_max;
> -
> -              n = __readlink (rpath, buf, path_max - 1);
> -              if (n < 0)
> -                goto error;
> +              if (n < sizeof linkbuf)
> +                buf = linkbuf;
> +              else
> +                {
> +                  buf = extra_buf + path_max;
> +                  n = __readlink (rpath, buf, path_max - 1);
> +                  if (n < 0)
> +                    goto error;
> +                }
>                buf[n] = '\0';
>  
>                len = strlen (end);
> @@ -350,11 +342,6 @@ __realpath (const char *name, char *resolved)
>                      dest++;
>                  }
>              }
> -          else if (!S_ISDIR (st.st_mode) && *end != '\0')
> -            {
> -              __set_errno (ENOTDIR);
> -              goto error;
> -            }
>          }
>      }
>    if (dest > rpath + prefix_len + 1 && ISSLASH (dest[-1]))
> diff --git a/modules/canonicalize-lgpl b/modules/canonicalize-lgpl
> index 20ee7908b..701b492db 100644
> --- a/modules/canonicalize-lgpl
> +++ b/modules/canonicalize-lgpl
> @@ -14,12 +14,10 @@ alloca-opt        [test $HAVE_CANONICALIZE_FILE_NAME = 0 
> || test $REPLACE_CANONI
>  double-slash-root [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  errno             [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  filename          [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
> -lstat             [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  malloca           [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  memmove           [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  pathmax           [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  readlink          [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
> -sys_stat          [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test 
> $REPLACE_CANONICALIZE_FILE_NAME = 1]
>  
>  configure.ac:
>  gl_CANONICALIZE_LGPL
>