Re: PATCH: realpath() and dead code

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Sat, Nov 29, 2014 at 10:08 AM, Oliver Kiddle <okiddle@xxxxxxxxxxx> wrote:
> Coverity reported dead code in chrealpath(). A past fix to bail out
> early and avoid a null pointer dereference was skipping the code that
> tries to repeat realpath/canonicalize_file_name with successive trailing
> parts of the path name lopped off until it succeeds. The !real test was
> actually tautologous.
>
> I've also removed ELOOP and ENAMETOOLONG from the errors that cause
> a bail out: I think it is useful to resolve symlinks in directories
> further up the path even if you have cyclic symlinks at the end.
>
> Finally, it seems FreeBSD is one modern system that accepts a
> NULL argument to realpath() but lacks the horrendously named
> canonicalize_file_name() alternative. So I've added an autoconf test for
> that instead. It still checks for canonicalize_file_name but only to
> provide a best guess in the case of a cross-compiler.
>
> There doesn't seem to be explicit test cases of :A. I'm not adding any
> because I suspect they'd be the sort of thing that fails regularly for
> unrelated reasons and because I don't have a clue how symlinks would
> work on odd systems like Cygwin and Mac OS.
>
> Oliver
>
> diff --git a/Src/hist.c b/Src/hist.c
> index 0831756..7fe843a 100644
> --- a/Src/hist.c
> +++ b/Src/hist.c
> @@ -1702,11 +1702,12 @@ int
>  chrealpath(char **junkptr)
>  {
>      char *str;
> -#ifdef HAVE_CANONICALIZE_FILE_NAME
> +#ifdef HAVE_REALPATH
> +# ifdef REALPATH_ACCEPTS_NULL
>      char *lastpos, *nonreal, *real;
> -#else
> -# ifdef HAVE_REALPATH
> -    char *lastpos, *nonreal, real[PATH_MAX];
> +# else
> +    char *lastpos, *nonreal, pathbuf[PATH_MAX];
> +    char *real = pathbuf;
>  # endif
>  #endif
>
> @@ -1717,7 +1718,7 @@ chrealpath(char **junkptr)
>      if (!chabspath(junkptr))
>         return 0;
>
> -#if !defined(HAVE_REALPATH) && !defined(HAVE_CANONICALIZE_FILE_NAME)
> +#ifndef HAVE_REALPATH
>      return 1;
>  #else
>      /*
> @@ -1733,29 +1734,21 @@ chrealpath(char **junkptr)
>      nonreal = lastpos + 1;
>
>      while (!
> -#ifdef HAVE_CANONICALIZE_FILE_NAME
> -          /*
> -           * This is a GNU extension to realpath(); it's the
> -           * same as calling realpath() with a NULL second argument
> -           * which uses malloc() to get memory.  The alternative
> -           * interface is easier to test for, however.
> -           */
> -          (real = canonicalize_file_name(*junkptr))
> +#ifdef REALPATH_ACCEPTS_NULL
> +          /* realpath() with a NULL second argument uses malloc() to get
> +           * memory so we don't need to worry about overflowing PATH_MAX */
> +          (real = realpath(*junkptr, NULL))
>  #else
>            realpath(*junkptr, real)
>  #endif
>         ) {
> -       if (errno == EINVAL || errno == ELOOP ||
> -           errno == ENAMETOOLONG || errno == ENOMEM)
> -           return 0;
> -
> -#ifdef HAVE_CANONICALIZE_FILE_NAME
> -       if (!real)
> +       if (errno == EINVAL || errno == ENOMEM)
>             return 0;
> -#endif
>
>         if (nonreal == *junkptr) {
> -           *real = '\0';
> +#ifndef REALPATH_ACCEPTS_NULL
> +           real = NULL;
> +#endif
>             break;
>         }
>
> @@ -1771,11 +1764,15 @@ chrealpath(char **junkptr)
>         str++;
>      }
>
> -    *junkptr = metafy(str = bicat(real, nonreal), -1, META_HEAPDUP);
> -    zsfree(str);
> -#ifdef HAVE_CANONICALIZE_FILE_NAME
> -    free(real);
> +    if (real) {
> +       *junkptr = metafy(str = bicat(real, nonreal), -1, META_HEAPDUP);
> +       zsfree(str);
> +#ifdef REALPATH_ACCEPTS_NULL
> +       free(real);
>  #endif
> +    } else {
> +       *junkptr = metafy(nonreal, lastpos - nonreal + 1, META_HEAPDUP);
> +    }
>  #endif
>
>      return 1;
> diff --git a/configure.ac b/configure.ac
> index 56c4cfb..8ea9737 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1302,6 +1302,23 @@ AC_CHECK_FUNCS(strftime strptime mktime timelocal \
>                cygwin_conv_path)
>  AC_FUNC_STRCOLL
>
> +AH_TEMPLATE([REALPATH_ACCEPTS_NULL],
> +[Define if realpath() accepts NULL as its second argument.])
> +AC_CACHE_CHECK([if realpath accepts NULL],
> +zsh_cv_func_realpath_accepts_null,
> +[AC_RUN_IFELSE([AC_LANG_PROGRAM([
> +#include <stdlib.h>
> +#include <limits.h>
> +],[
> +exit(!realpath("/", (char*)0));
> +])],
> +[zsh_cv_func_realpath_accepts_null=yes],
> +[zsh_cv_func_realpath_accepts_null=no],
> +[zsh_cv_func_realpath_accepts_null=$ac_cv_func_canonicalize_file_name])])
> +if test x$zsh_cv_func_realpath_accepts_null = xyes; then
> +  AC_DEFINE(REALPATH_ACCEPTS_NULL)
> +fi
> +
>  if test x$enable_cap = xyes; then
>    AC_CHECK_FUNCS(cap_get_proc)
>  fi

This breaks cross-compilation, or requires manually figuring out the
answer and injecting it in the autoconf cache. (depending on your
definition of "break").

-- 
Mikael Magnusson