[Bug-tar] [PATCH] tar: no need to report getcwd error if never using the result

[Paul Eggert]

My recent commit "tar: don't crash if getcwd fails" inserted
the following code to chdir_do:

        if (! prev->saved_cwd.name)
          err = errno;

so that if xgetcwd fails, the error is reported immediately, rather
than a null pointer dereference and core dump later.  This is a
conservative patch, but in some cases it is too conservative, as in
some cases the null pointer is never used later and 'tar' would
complete successfully if it didn't report the getcwd failure now.

I created a test case to illustrate the problem, and pushed the
following patch to fix this.  The problem does not occur on my RHEL 5
host (due to the getcwd magic in this version of Linux) but it does
occur on Solaris 10 (which uses different magic, that happens to not
work in this case).

>From 741e6abe995ad659992bdcc2e8c7a56ed3a6488a Mon Sep 17 00:00:00 2001
From: Paul R. Eggert <address@hidden>
Date: Sun, 18 Jul 2010 12:19:18 -0700
Subject: [PATCH] tar: no need to report getcwd error if never using the result

* src/misc.c (struct wd): Rename 'saved' to 'err', with new semantics.
(chdir_arg, chdir_do): Adjust to new semantics.  Do not report an
error merely because save_cwd fails; report an error only if
save_cwd's result is needed later.
* tests/extrac09.at: New file, to test for bug that was fixed.
* tests/testsuite.at: Include it.
* tests/Makefile.am (TESTSUITE_AT): Add it.
---
 src/misc.c         |   36 +++++++++++++++++++++---------------
 tests/Makefile.am  |    1 +
 tests/extrac09.at  |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/testsuite.at |    1 +
 4 files changed, 69 insertions(+), 15 deletions(-)
 create mode 100644 tests/extrac09.at

diff --git a/src/misc.c b/src/misc.c
index 40635be..34fa6e6 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -640,8 +640,15 @@ set_file_atime (int fd, char const *file, struct timespec 
const timespec[2])
 /* A description of a working directory.  */
 struct wd
 {
+  /* The directory's name.  */
   char const *name;
-  int saved;
+
+  /* A negative value if no attempt has been made to save the
+     directory, 0 if it was saved successfully, and a positive errno
+     value if it was not saved successfully.  */
+  int err;
+
+  /* The saved version of the directory, if SAVED == 1.  */
   struct saved_cwd saved_cwd;
 };
 
@@ -680,7 +687,7 @@ chdir_arg (char const *dir)
       if (! wd_count)
        {
          wd[wd_count].name = ".";
-         wd[wd_count].saved = 0;
+         wd[wd_count].err = -1;
          wd_count++;
        }
     }
@@ -697,7 +704,7 @@ chdir_arg (char const *dir)
     }
 
   wd[wd_count].name = dir;
-  wd[wd_count].saved = 0;
+  wd[wd_count].err = -1;
   return wd_count++;
 }
 
@@ -713,12 +720,11 @@ chdir_do (int i)
       struct wd *prev = &wd[previous];
       struct wd *curr = &wd[i];
 
-      if (! prev->saved)
+      if (prev->err < 0)
        {
-         int err = 0;
-         prev->saved = 1;
+         prev->err = 0;
          if (save_cwd (&prev->saved_cwd) != 0)
-           err = errno;
+           prev->err = errno;
          else if (0 <= prev->saved_cwd.desc)
            {
              /* Make sure we still have at least one descriptor available.  */
@@ -733,20 +739,20 @@ chdir_do (int i)
                  prev->saved_cwd.desc = -1;
                  prev->saved_cwd.name = xgetcwd ();
                  if (! prev->saved_cwd.name)
-                   err = errno;
+                   prev->err = errno;
                }
              else
-               err = errno;
+               prev->err = errno;
            }
-
-         if (err)
-           FATAL_ERROR ((0, err, _("Cannot save working directory")));
        }
 
-      if (curr->saved)
+      if (0 <= curr->err)
        {
-         if (restore_cwd (&curr->saved_cwd))
-           FATAL_ERROR ((0, 0, _("Cannot change working directory")));
+         int err = curr->err;
+         if (err == 0 && restore_cwd (&curr->saved_cwd) != 0)
+           err = errno;
+         if (err)
+           FATAL_ERROR ((0, err, _("Cannot restore working directory")));
        }
       else
        {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5232adb..01b0bae 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -75,6 +75,7 @@ TESTSUITE_AT = \
  extrac06.at\
  extrac07.at\
  extrac08.at\
+ extrac09.at\
  filerem01.at\
  filerem02.at\
  gzip.at\
diff --git a/tests/extrac09.at b/tests/extrac09.at
new file mode 100644
index 0000000..4506c04
--- /dev/null
+++ b/tests/extrac09.at
@@ -0,0 +1,46 @@
+# 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([no need to save dir with unreadable . and ..])
+AT_KEYWORDS([extract extrac09])
+
+AT_TAR_CHECK([
+mkdir dir
+mkdir dir/sub
+mkdir dir/sub/extract
+genfile --file dir/sub/f
+cd dir/sub
+
+tar -cf archive.tar f
+
+chmod a-r . ..
+tar -xvf archive.tar -C extract f
+status=$?
+chmod a+r . ..
+cmp f extract/f || status=$?
+exit $status
+],
+[0],
+[f
+],
+[],[],[],[gnu])
+
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 6edcf4e..1048a0a 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -146,6 +146,7 @@ m4_include([extrac05.at])
 m4_include([extrac06.at])
 m4_include([extrac07.at])
 m4_include([extrac08.at])
+m4_include([extrac09.at])
 
 m4_include([label01.at])
 m4_include([label02.at])
-- 
1.7.1