PATCH: realpath() and dead code

PATCH: realpath() and dead code

[Oliver Kiddle]

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

Re: PATCH: realpath() and dead code

[Mikael Magnusson]

On Sat, Nov 29, 2014 at 10:08 AM, Oliver Kiddle <okiddle@yahoo.co.uk> 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

Re: PATCH: realpath() and dead code

[Oliver Kiddle]

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

As I mentioned, it uses the existance or otherwise of
canonicalize_file_name in the action-if-cross-compiling part. (I'm not
aware of any system that has canonicalize_file_name but doesn't accept
NULL).

Even with a cross-compile action of zsh_cv_func_realpath_accepts_null=no
you won't have a broken build, just a suboptimal one. And without the
patch you currently get a similarly suboptimal build on FreeBSD when
compiling natively. Same goes for all the other BSDs including Darwin
judging by online man pages.

Aside from that, there's already 23 instances of AC_TRY_RUN in
configure.ac so compromising cross-compilation is surely not new or
unique to this. Do you regularly cross-compile zsh? Is this really
making a difference that wasn't there before? Or do you perhaps have
another suggestion? The only other way I can think of is to explicitly
#ifdef known systems. Anyone else got any thoughts?

Oliver