path_concat proposed cleanup for corutils

[Paul Eggert]

I wanted to use path_concat in Bison to fix a porting problem to DOS,
and noticed that path_concat had the same problem: it mishandled the
case where BASE starts with "c:".  While fixing this, I noticed a few
other problems:

* cp, install, and ls all assume that path_concat never returns NULL,
  and they can dump core if path_concat fails due to memory exhaustion.

* There is a function xpath_concat defined in path-concat.c but it's not
  declared and nobody uses it.  I think it's simpler to have path_concat
  always return a nonnull pointer.

* cp.c has a potential integer overflow if the dir name has more than 2 GiB,
  and will trash the resulting file name.

* Only nohup.c passes a NULL pointer as the first argument to
  path_concat, but it is a glitch, and it causes nohup to try to open
  the same file twice.  I rewrote nohup to avoid the glitch, so that
  path_concat can now assume that its first argument is non NULL.

* There is a (perhaps theoretical?) problem if some standard header
  defines mempcpy as a macro, but there is no mempcpy function.

* Sometimes more than one slash is inserted between the dir and the base,
  if they started with multiple slashes there.  This is just a minor annoyance
  but while we're in the neighborhood....

Here is a patch.

Index: ChangeLog
===================================================================
RCS file: /home/meyering/coreutils/cu/ChangeLog,v
retrieving revision 1.976
diff -p -u -r1.976 ChangeLog
--- ChangeLog   2 Jul 2004 17:01:30 -0000       1.976
+++ ChangeLog   2 Jul 2004 21:02:30 -0000
@@ -1,3 +1,16 @@
+2004-07-02  Paul Eggert  <address@hidden>
+
+       * src/copy.c (copy_dir): Assume path_concat returns non-NULL.
+       * src/cp.c (do_copy): Likewise.
+       * src/mv.c (movefile): Likewise.
+
+       * src/cp.c (make_path_private): 2nd arg is now size_t, not int,
+       to avoid problem when path_concat dir name is longer than 2 GiB (!).
+
+       * src/nohup.c (main): Don't pass NULL first argument to path_concat.
+       This cleans up the semantics a bit, as we no longer try to open the
+       same file twice.
+
 2004-07-01  Paul Eggert  <address@hidden>
 
        * Version 5.3.0.
Index: lib/ChangeLog
===================================================================
RCS file: /home/meyering/coreutils/cu/lib/ChangeLog,v
retrieving revision 1.783
diff -p -u -r1.783 ChangeLog
--- lib/ChangeLog       30 Jun 2004 22:40:52 -0000      1.783
+++ lib/ChangeLog       2 Jul 2004 20:58:03 -0000
@@ -1,3 +1,16 @@
+2004-07-02  Paul Eggert  <address@hidden>
+
+       * canonicalize.c (canonicalize_file_name): Assume that path_concat
+       never returns NULL.
+       * path-concat.c (mempcpy): Don't define if a system header defines it.
+       Don't include stdio.h, stdlib.h, unistd.h, strdup.h.
+       (longest_relative_suffix): New function.
+       (path_concat): Use it.  Assume first argument is not NULL.
+       Port to DOS.  Omit redundant separators.
+       Report an error instead of returning NULL.
+       Use mempcpy instead of memcpy.
+       (xpath_concat): Remove: not declared or used.
+       
 2004-06-30  Paul Eggert  <address@hidden>
 
        * dirname.h (FILE_SYSTEM_PREFIX_LEN): Renamed from
Index: lib/canonicalize.c
===================================================================
RCS file: /home/meyering/coreutils/cu/lib/canonicalize.c,v
retrieving revision 1.12
diff -p -u -r1.12 canonicalize.c
--- lib/canonicalize.c  19 Jun 2004 12:28:02 -0000      1.12
+++ lib/canonicalize.c  30 Jun 2004 18:23:22 -0000
@@ -129,9 +129,6 @@ canonicalize_file_name (const char *name
        return NULL;
 
       extra_buf = path_concat (wd, name, NULL);
-      if (!extra_buf)
-       xalloc_die ();
-
       name = extra_buf;
       free (wd);
     }
Index: lib/path-concat.c
===================================================================
RCS file: /home/meyering/coreutils/cu/lib/path-concat.c,v
retrieving revision 1.19
diff -p -u -r1.19 path-concat.c
--- lib/path-concat.c   30 Jun 2004 22:40:47 -0000      1.19
+++ lib/path-concat.c   2 Jul 2004 20:53:33 -0000
@@ -26,85 +26,62 @@
 /* Specification.  */
 #include "path-concat.h"
 
-#ifndef HAVE_MEMPCPY
-# define mempcpy(D, S, N) ((void *) ((char *) memcpy (D, S, N) + (N)))
-#endif
-
-#include <stdio.h>
-#include <stdlib.h>
 #include <string.h>
 
-#if HAVE_UNISTD_H
-# include <unistd.h>
-#endif
-
-#include "strdup.h"
 #include "dirname.h"
 #include "xalloc.h"
 
-/* Concatenate two pathname components, DIR and BASE, in
-   newly-allocated storage and return the result.  Return 0 if out of
-   memory.  Add a slash between DIR and BASE in the result if neither
-   would contribute one.  If each would contribute at least one, elide
-   one from the end of DIR.  Otherwise, simply concatenate DIR and
-   BASE.  In any case, if BASE_IN_RESULT is non-NULL, set
+#if ! HAVE_MEMPCPY && ! defined mempcpy
+# define mempcpy(D, S, N) ((void *) ((char *) memcpy (D, S, N) + (N)))
+#endif
+
+/* Return the longest suffix of F that is a relative file name.
+   If it has no such suffix, return the empty string.  */
+
+static char const *
+longest_relative_suffix (char const *f)
+{
+  for (f += FILE_SYSTEM_PREFIX_LEN (f); ISSLASH (*f); f++)
+    continue;
+  return f;
+}
+
+/* Concatenate two pathname components, DIR and ABASE, in
+   newly-allocated storage and return the result.
+   The resulting file name is equivalent to what you would get by
+   running (cd DIR; cat BASE), where BASE is ABASE with any file system
+   prefixes and leading separators removed.
+   Arrange for a directory separator if necessary between DIR and BASE
+   in the result, removing any redundant separators.
+   In any case, if BASE_IN_RESULT is non-NULL, set
    *BASE_IN_RESULT to point to the copy of BASE in the returned
    concatenation.
 
-   DIR may be NULL, BASE must not be.
-
-   Return NULL if memory is exhausted.  */
+   Report an error if memory is exhausted.  */
 
 char *
-path_concat (const char *dir, const char *base, char **base_in_result)
+path_concat (char const *dir, char const *abase, char **base_in_result)
 {
+  char const *dirbase = base_name (dir);
+  size_t dirbaselen = base_len (dirbase);
+  size_t dirlen = dirbase - dir + dirbaselen;
+  size_t needs_separator = (dirbaselen && ! ISSLASH (dirbase[dirbaselen - 1]));
+
+  char const *base = longest_relative_suffix (abase);
+  size_t baselen = strlen (base);
+
+  char *p_concat = xmalloc (dirlen + needs_separator + baselen + 1);
   char *p;
-  char *p_concat;
-  size_t baselen;
-  size_t dirlen;
-
-  if (!dir)
-    {
-      p_concat = strdup (base);
-      if (base_in_result)
-        *base_in_result = p_concat;
-      return p_concat;
-    }
-
-  /* DIR is not empty. */
-  baselen = base_len (base);
-  dirlen = strlen (dir);
-
-  p_concat = malloc (dirlen + baselen + 2);
-  if (!p_concat)
-    return 0;
 
   p = mempcpy (p_concat, dir, dirlen);
-
-  if (FILE_SYSTEM_PREFIX_LEN (dir) < dirlen)
-    {
-      if (ISSLASH (*(p - 1)) && ISSLASH (*base))
-       --p;
-      else if (!ISSLASH (*(p - 1)) && !ISSLASH (*base))
-       *p++ = DIRECTORY_SEPARATOR;
-    }
+  *p = DIRECTORY_SEPARATOR;
+  p += needs_separator;
 
   if (base_in_result)
     *base_in_result = p;
 
-  memcpy (p, base, baselen);
-  p[baselen] = '\0';
+  p = mempcpy (p, base, baselen);
+  *p = '\0';
 
   return p_concat;
-}
-
-/* Same, but die when memory is exhausted. */
-
-char *
-xpath_concat (const char *dir, const char *base, char **base_in_result)
-{
-  char *res = path_concat (dir, base, base_in_result);
-  if (! res)
-    xalloc_die ();
-  return res;
 }
Index: m4/ChangeLog
===================================================================
RCS file: /home/meyering/coreutils/cu/m4/ChangeLog,v
retrieving revision 1.588
diff -p -u -r1.588 ChangeLog
--- m4/ChangeLog        30 Jun 2004 22:44:15 -0000      1.588
+++ m4/ChangeLog        2 Jul 2004 20:59:18 -0000
@@ -1,3 +1,9 @@
+2004-07-02  Paul Eggert  <address@hidden>
+
+       * path-concat.m4 (gl_PATH_CONCAT): Don't require gl_AC_DOS, the
+       prerequisite modules now handle the DOS stuff.
+       Don't check for unistd.h.
+       
 2004-06-30  Paul Eggert  <address@hidden>
 
        * dos.m4 (gl_AC_DOS): Define FILE_SYSTEM_PREFIX_LEN, not
Index: m4/path-concat.m4
===================================================================
RCS file: /home/meyering/coreutils/cu/m4/path-concat.m4,v
retrieving revision 1.3
diff -p -u -r1.3 path-concat.m4
--- m4/path-concat.m4   13 Apr 2004 15:28:45 -0000      1.3
+++ m4/path-concat.m4   2 Jul 2004 20:45:21 -0000
@@ -1,4 +1,4 @@
-# path-concat.m4 serial 3
+# path-concat.m4 serial 4
 dnl Copyright (C) 2002, 2003, 2004 Free Software Foundation, Inc.
 dnl This file is free software, distributed under the terms of the GNU
 dnl General Public License.  As a special exception to the GNU General
@@ -9,7 +9,5 @@ dnl the same distribution terms as the r
 AC_DEFUN([gl_PATH_CONCAT],
 [
   dnl Prerequisites of lib/path-concat.c.
-  AC_REQUIRE([gl_AC_DOS])
-  AC_CHECK_HEADERS_ONCE(unistd.h)
   AC_CHECK_FUNCS_ONCE(mempcpy)
 ])
Index: src/copy.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/copy.c,v
retrieving revision 1.166
diff -p -u -r1.166 copy.c
--- src/copy.c  30 Jun 2004 22:33:40 -0000      1.166
+++ src/copy.c  2 Jul 2004 17:49:08 -0000
@@ -172,9 +172,6 @@ copy_dir (const char *src_path_in, const
       char *src_path = path_concat (src_path_in, namep, NULL);
       char *dst_path = path_concat (dst_path_in, namep, NULL);
 
-      if (dst_path == NULL || src_path == NULL)
-       xalloc_die ();
-
       ret |= copy_internal (src_path, dst_path, new_dst, src_sb->st_dev,
                            ancestors, &non_command_line_options, 0,
                            &local_copy_into_self, NULL);
Index: src/cp.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/cp.c,v
retrieving revision 1.198
diff -p -u -r1.198 cp.c
--- src/cp.c    2 Jul 2004 17:00:10 -0000       1.198
+++ src/cp.c    2 Jul 2004 18:19:07 -0000
@@ -371,7 +371,7 @@ re_protect (const char *const_dst_path, 
 /* FIXME: find a way to synch this function with the one in lib/makepath.c. */
 
 static int
-make_path_private (const char *const_dirpath, int src_offset, int mode,
+make_path_private (const char *const_dirpath, size_t src_offset, int mode,
                   const char *verbose_fmt_string, struct dir_attr **attr_list,
                   int *new_dst, int (*xstat)())
 {
@@ -582,8 +582,6 @@ do_copy (int n_files, char **file, const
              /* Append all of `arg' (minus any trailing slash) to `dest'.  */
              dst_path = path_concat (target_directory, arg_no_trailing_slash,
                                      &arg_in_concat);
-             if (dst_path == NULL)
-               xalloc_die ();
 
              /* For --parents, we have to make sure that the directory
                 dir_name (dst_path) exists.  We may have to create a few
Index: src/mv.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/mv.c,v
retrieving revision 1.158
diff -p -u -r1.158 mv.c
--- src/mv.c    2 Jul 2004 17:01:20 -0000       1.158
+++ src/mv.c    2 Jul 2004 17:49:09 -0000
@@ -277,8 +277,6 @@ movefile (char *source, char *dest, bool
       /* Treat DEST as a directory; build the full filename.  */
       char const *src_basename = base_name (source);
       char *new_dest = path_concat (dest, src_basename, NULL);
-      if (new_dest == NULL)
-       xalloc_die ();
       strip_trailing_slashes (new_dest);
       fail = do_move (source, new_dest, x);
       free (new_dest);
Index: src/nohup.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/nohup.c,v
retrieving revision 1.15
diff -p -u -r1.15 nohup.c
--- src/nohup.c 21 Jun 2004 15:03:35 -0000      1.15
+++ src/nohup.c 2 Jul 2004 18:33:20 -0000
@@ -109,19 +109,24 @@ main (int argc, char **argv)
       char const *file = "nohup.out";
       int flags = O_CREAT | O_WRONLY | O_APPEND;
       mode_t mode = S_IRUSR | S_IWUSR;
-      int saved_errno;
 
       fd = open (file, flags, mode);
       if (fd == -1)
        {
-         saved_errno = errno;
-         in_home = path_concat (getenv ("HOME"), file, NULL);
-         fd = open (in_home, flags, mode);
+         int saved_errno = errno;
+         char const *home = getenv ("HOME");
+         if (home)
+           {
+             in_home = path_concat (home, file, NULL);
+             fd = open (in_home, flags, mode);
+           }
          if (fd == -1)
            {
              int saved_errno2 = errno;
              error (0, saved_errno, _("failed to open %s"), quote (file));
-             error (0, saved_errno2, _("failed to open %s"), quote (in_home));
+             if (in_home)
+               error (0, saved_errno2, _("failed to open %s"),
+                      quote (in_home));
              exit (NOHUP_FAILURE);
            }
          file = in_home;