Re: [Bug-tar] segfault getting cwd with --listed-incremental

[Paul Eggert]

On 06/02/10 09:26, J Chapman Flack wrote:

> There could be discussion of what the bug really is. For one
> thing, why does --listed-incremental even want to get the cwd?

I haven't a clue.  Let's remove that.  I tried doing that in the patch
enclosed below, which I installed.  Sergey, can you please
double-check this?  Was there some reason those path names had to be
absolute?  Anyway, the patched version passes the tests so if there is
some reason the path names have to be absolute, that should be tested
for.

> "one should not absolutize without necessity" seems like
> a sound principle.

Quite true.  After the patch below, tar invokes getcwd only when it
might need to chdir back to the working directory and can't open the
working directory for reading (and thus can't do it with fchdir).
Even this could be fixed (on Linux and Solaris, anyway) if tar would
be improved to prefer openat to fchdir+open.  At some point tar should
prefer openat anyway, as that also solves some path-length and
tree-walking problems; but this is a much bigger project.

> --listed-incremental to need the cwd, it's common these days for
> the OS to provide getcwd as a syscall to avoid the failure modes
> of the old algorithm.

Also true, and worth doing even assuming the patch enclosed below.
I addressed this with a separate patch to gnulib
<http://lists.gnu.org/archive/html/bug-gnulib/2010-07/msg00095.html>
and this should be picked up automatically when tar syncs to the
latest gnulib.

Thanks for your bug report and the clear discussion of the issues.
I hope this patch resolves things satisfactorily; if not, please
follow up to <address@hidden>.

>From cc40c57a37b1e443a5088fdda0fbb3687b1f4773 Mon Sep 17 00:00:00 2001
From: Paul R. Eggert <address@hidden>
Date: Thu, 15 Jul 2010 11:24:39 -0700
Subject: [PATCH] tar: don't crash if getcwd fails

* src/extract.c: Don't include xgetcwd.h.
(extract_dir): stat "." rather than statting getcwd's output.
* src/misc.c (normalize_filename_x): Rewrite so as not to resolve
/../, which can't be done reliably in the presence of symlinks.
Don't reject valid names such as ".".
(normalize_filename): Don't make it absolute; that way, we don't
have to invoke xgetcwd which might fail.  Don't bother to realloc
at the end, since that uses time and now saves little space.
(chdir_do): Don't crash if xgetcwd fails.
* tests/Makefile.am (TESTSUITE_AT): Add listed03.at.
* tests/listed03.at: New file.
* tests/testsuite.at: Include listed03.at.
---
 src/extract.c      |    7 +--
 src/misc.c         |  127 +++++++++++++++++-----------------------------------
 tests/Makefile.am  |    3 +-
 tests/listed03.at  |   45 ++++++++++++++++++
 tests/testsuite.at |    5 +-
 5 files changed, 94 insertions(+), 93 deletions(-)
 create mode 100644 tests/listed03.at

diff --git a/src/extract.c b/src/extract.c
index 7ce9ce8..9897e82 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -23,7 +23,6 @@
 #include <quotearg.h>
 #include <utimens.h>
 #include <errno.h>
-#include <xgetcwd.h>
 #include <priv-set.h>
 
 #include "common.h"
@@ -648,13 +647,11 @@ extract_dir (char *file_name, int typeflag)
   if (one_file_system_option && root_device == 0)
     {
       struct stat st;
-      char *dir = xgetcwd ();
 
-      if (deref_stat (true, dir, &st))
-       stat_diag (dir);
+      if (stat (".", &st) != 0)
+       stat_diag (".");
       else
        root_device = st.st_dev;
-      free (dir);
     }
 
   if (incremental_option)
diff --git a/src/misc.c b/src/misc.c
index 3e7941d..12b40ac 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -1,7 +1,7 @@
 /* Miscellaneous functions, not really specific to GNU tar.
 
    Copyright (C) 1988, 1992, 1994, 1995, 1996, 1997, 1999, 2000, 2001,
-   2003, 2004, 2005, 2006, 2007, 2009 Free Software Foundation, Inc.
+   2003, 2004, 2005, 2006, 2007, 2009, 2010 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
@@ -233,99 +233,54 @@ zap_slashes (char *name)
   return name;
 }
 
-/* Normalize NAME by resolving any relative references and
-   removing trailing slashes.  Destructive version: modifies its argument. */
-static int
-normalize_filename_x (char *name)
-{
-  char *p, *q;
-
-  p = name;
-  if (DOUBLE_SLASH_IS_DISTINCT_ROOT && ISSLASH (*p))
-    p++;
-
-  /* Remove /./, resolve /../ and compress sequences of slashes */
-  for (q = p; *q; )
+/* Normalize FILE_NAME by removing redundant slashes and "."
+   components, including redundant trailing slashes.  Leave ".."
+   alone, as it may be significant in the presence of symlinks and on
+   platforms where "/.." != "/".  Destructive version: modifies its
+   argument. */
+static void
+normalize_filename_x (char *file_name)
+{
+  char *name = file_name + FILE_SYSTEM_PREFIX_LEN (file_name);
+  char *p;
+  char const *q;
+  char c;
+
+  /* Don't squeeze leading "//" to "/", on hosts where they're distinct.  */
+  name += (DOUBLE_SLASH_IS_DISTINCT_ROOT
+          && ISSLASH (*name) && ISSLASH (name[1]) && ! ISSLASH (name[2]));
+
+  /* Omit redundant leading "." components.  */
+  for (q = p = name; (*p = *q) == '.' && ISSLASH (q[1]); p += !*q)
+    for (q += 2; ISSLASH (*q); q++)
+      continue;
+
+  /* Copy components from Q to P, omitting redundant slashes and
+     internal "."  components.  */
+  while ((*p++ = c = *q++) != '\0')
+    if (ISSLASH (c))
+      while (ISSLASH (q[*q == '.']))
+       q += (*q == '.') + 1;
+
+  /* Omit redundant trailing "." component and slash.  */
+  if (2 < p - name)
     {
-      if (ISSLASH (*q))
-       {
-         *p++ = *q++;
-         while (ISSLASH (*q))
-           q++;
-         continue;
-       }
-      else if (p == name)
-       {
-         if (*q == '.')
-           {
-             if (ISSLASH (q[1]))
-               {
-                 q += 2;
-                 continue;
-               }
-             if (q[1] == '.' && ISSLASH (q[2]))
-               return 1;
-           }
-       }
-      else
-       {
-         if (*q == '.' && ISSLASH (p[-1]))
-           {
-             if (ISSLASH (q[1]))
-               {
-                 q += 2;
-                 while (ISSLASH (*q))
-                   q++;
-                 continue;
-               }
-             else if (q[1] == '.' && ISSLASH (q[2]))
-               {
-                 do
-                   {
-                     --p;
-                   }
-                 while (p > name && !ISSLASH (p[-1]));
-                 q += 3;
-                 continue;
-               }
-           }
-       }
-      *p++ = *q++;
+      p -= p[-2] == '.' && ISSLASH (p[-3]);
+      p -= 2 < p - name && ISSLASH (p[-2]);
+      p[-1] = '\0';
     }
-
-  /* Remove trailing slashes */
-  while (p - 1 > name && ISSLASH (p[-1]))
-    p--;
-
-  *p = 0;
-  return 0;
 }
 
-/* Normalize NAME by resolving any relative references, removing trailing
-   slashes, and converting it to absolute file name.  Return the normalized
-   name, or NULL in case of error. */
+/* Normalize NAME by removing redundant slashes and "." components,
+   including redundant trailing slashes.  Return a normalized
+   newly-allocated copy.  */
 
 char *
 normalize_filename (const char *name)
 {
-  char *copy;
-
-  if (name[0] != '/')
-    {
-      copy = xgetcwd ();
-      copy = xrealloc (copy, strlen (copy) + strlen (name) + 2);
-
-      strcat (copy, "/");
-      strcat (copy, name);
-    }
-  else
-    copy = xstrdup (name);
-  if (normalize_filename_x (copy))
-    {
-      free (copy);
-      return NULL;
-    }
-  return xrealloc (copy, strlen (copy) + 1);
+  char *copy = xstrdup (name);
+  normalize_filename_x (copy);
+  return copy;
 }
 
 
@@ -752,6 +707,8 @@ chdir_do (int i)
                  close (fd1);
                  prev->saved_cwd.desc = -1;
                  prev->saved_cwd.name = xgetcwd ();
+                 if (! prev->saved_cwd.name)
+                   err = errno;
                }
              else
                err = errno;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f5bc4ef..5232adb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,6 +1,6 @@
 # Makefile for GNU tar regression tests.
 
-# Copyright (C) 1996, 1997, 1999, 2000, 2001, 2003, 2004, 2005, 
+# Copyright (C) 1996, 1997, 1999, 2000, 2001, 2003, 2004, 2005,
 # 2006, 2007, 2009 Free Software Foundation, Inc.
 
 # François Pinard <address@hidden>, 1988.
@@ -98,6 +98,7 @@ TESTSUITE_AT = \
  link03.at\
  listed01.at\
  listed02.at\
+ listed03.at\
  long01.at\
  longv7.at\
  lustar01.at\
diff --git a/tests/listed03.at b/tests/listed03.at
new file mode 100644
index 0000000..5bf8e58
--- /dev/null
+++ b/tests/listed03.at
@@ -0,0 +1,45 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+
+# Test suite for GNU tar.
+# Copyright (C) 2010 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, 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 <http://www.gnu.org/licenses/>.
+
+# This checks for the --listed-incremental bug reported by J Chapman Flack at
+# http://lists.gnu.org/archive/html/bug-tar/2010-06/msg00000.html
+
+AT_SETUP([incremental dump when the parent directory is unreadable])
+AT_KEYWORDS([listed incremental listed03])
+
+AT_TAR_CHECK([
+mkdir dir
+mkdir dir/sub
+mkdir dir/sub/a
+genfile --file dir/sub/a/file
+cd dir/sub
+
+chmod a-r ..
+tar -c -f archive.tar --listed-incremental=db.1 -v a
+status=$?
+chmod a+r ..
+exit $status
+],
+[0],
+[a/
+a/file
+],
+[tar: a: Directory is new
+],[],[],[gnu])
+
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 37c1907..6edcf4e 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -41,13 +41,13 @@ $1)],$2,$3,$4,$5,$6)
 m4_define([AT_TAR_WITH_HOOK],[
   m4_pushdef([AT_TAR_CHECK_HOOK],[$1])
   $2
-  
+
   m4_popdef([AT_TAR_CHECK_HOOK])])
 
 m4_define([TAR_IGNREC_HOOK],[
   AT_CHECK([grep -v '^.*tar: Record size = ' stderr; exit 0])
 ])
-  
+
 m4_define([RE_CHECK],[
 AT_DATA([$1.re],[$2])
 awk '{print NR " " $[]0}' $1 > $[]$.1
@@ -162,6 +162,7 @@ m4_include([incr01.at])
 m4_include([incr02.at])
 m4_include([listed01.at])
 m4_include([listed02.at])
+m4_include([listed03.at])
 m4_include([incr03.at])
 m4_include([incr04.at])
 m4_include([incr05.at])
-- 
1.7.1