Re: [Bug-tar] --backup can destroy an extracted member when followed by an existing directory

2009-08-05 Thread Sergey Poznyakoff
Carl Worth cwo...@cworth.org ha escrit:

 I've traced through the code a bit and the problem is due to the
 handling of directory entries in extract.c:extract_archive().

Thanks for the in-depth analysis. I have applied the attached patch.

Regards,
Sergey

From 755c246588092d0b281cb610a8371c2c9b32de59 Mon Sep 17 00:00:00 2001
From: Sergey Poznyakoff g...@gnu.org.ua
Date: Wed, 5 Aug 2009 10:38:50 +0300
Subject: [PATCH] Fix backup handling and restoring file modes of existing 
directories

* NEWS, THANKS: Update
* src/extract.c (extract_dir): reset status to 0 if the
directory already exists.
* src/misc.c (maybe_backup_file): Assign before_backup_name
and clear after_backup_name before checking if we really need
to backup the file.
* tests/backup01.at: New testcase.
* tests/extrac08.at: New testcase.
* tests/Makefile.am, tests/testsuite.at: Add extrac08.at and
backup01.at
---
 NEWS   |8 +++-
 THANKS |1 +
 src/extract.c  |1 +
 src/misc.c |   17 +
 tests/Makefile.am  |2 ++
 tests/backup01.at  |   49 +
 tests/extrac08.at  |   51 +++
 tests/testsuite.at |3 +++
 8 files changed, 123 insertions(+), 9 deletions(-)
 create mode 100644 tests/backup01.at
 create mode 100644 tests/extrac08.at

diff --git a/NEWS b/NEWS
index e9a31ca..9c15ad7 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,4 @@
-GNU tar NEWS - User visible changes. 2009-05-25
+GNU tar NEWS - User visible changes. 2009-08-05
 Please send GNU tar bug reports to bug-tar@gnu.org
 
 
@@ -10,6 +10,12 @@ When listing or extracting archives, the actual record size 
is
 reported only if the archive is read from a device (as opposed
 to regular files and pipes).
 
+* Bugfixes
+** Fix handling of hard link targets by -c --transform.
+** Fix hard links recognition with -c --remove-files.
+** Fix restoring files from backup (debian bug #508199).
+** Correctly restore modes and permissions on existing directories.
+
 
 version 1.22 - Sergey Poznyakoff, 2009-03-05
 
diff --git a/THANKS b/THANKS
index dbb64d2..9d918ba 100644
--- a/THANKS
+++ b/THANKS
@@ -70,6 +70,7 @@ Burkhard Plache   pla...@krusty.optimax.ns.ca
 Calvin Cliff   cl...@trifid.astro.ucla.edu
 Cameron Elliottc...@mvbms.mvbms.com
 Carl Streeter  stree...@cae.wisc.edu
+Carl Worth  cwo...@cworth.org
 Carsten Heyl   h...@nads.de
 Catrin Urbanneck   c...@gppc.de
 Cesar Romani   rom...@ifm.uni-hamburg.de
diff --git a/src/extract.c b/src/extract.c
index 63f3525..5361506 100644
--- a/src/extract.c
+++ b/src/extract.c
@@ -682,6 +682,7 @@ extract_dir (char *file_name, int typeflag)
}
  if (S_ISDIR (st.st_mode))
{
+ status = 0;
  mode = st.st_mode;
  break;
}
diff --git a/src/misc.c b/src/misc.c
index 951449e..b609b86 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -417,6 +417,15 @@ maybe_backup_file (const char *file_name, bool 
this_is_the_archive)
 {
   struct stat file_stat;
 
+  assign_string (before_backup_name, file_name);
+
+  /* A run situation may exist between Emacs or other GNU programs trying to
+ make a backup for the same file simultaneously.  If theoretically
+ possible, real problems are unlikely.  Doing any better would require a
+ convention, GNU-wide, for all programs doing backups.  */
+
+  assign_string (after_backup_name, 0);
+
   /* Check if we really need to backup the file.  */
 
   if (this_is_the_archive  _remdev (file_name))
@@ -438,14 +447,6 @@ maybe_backup_file (const char *file_name, bool 
this_is_the_archive)
(S_ISBLK (file_stat.st_mode) || S_ISCHR (file_stat.st_mode)))
 return true;
 
-  assign_string (before_backup_name, file_name);
-
-  /* A run situation may exist between Emacs or other GNU programs trying to
- make a backup for the same file simultaneously.  If theoretically
- possible, real problems are unlikely.  Doing any better would require a
- convention, GNU-wide, for all programs doing backups.  */
-
-  assign_string (after_backup_name, 0);
   after_backup_name = find_backup_file_name (file_name, backup_type);
   if (! after_backup_name)
 xalloc_die ();
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2001834..64b1730 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -52,6 +52,7 @@ TESTSUITE_AT = \
  append.at\
  append01.at\
  append02.at\
+ backup01.at\
  chtype.at\
  comprec.at\
  delete01.at\
@@ -67,6 +68,7 @@ TESTSUITE_AT = \
  extrac05.at\
  extrac06.at\
  extrac07.at\
+ extrac08.at\
  gzip.at\
  grow.at\
  incremental.at\
diff --git a/tests/backup01.at b/tests/backup01.at
new file mode 100644
index 000..538dd3d
--- /dev/null
+++ b/tests/backup01.at
@@ -0,0 +1,49 @@
+# Process this file with autom4te to create testsuite. -*- Autotest -*-
+
+# Test suite for GNU tar.

[Bug-tar] --backup can destroy an extracted member when followed by an existing directory

2009-08-04 Thread Carl Worth
On Mon, 08 Dec 2008 13:41:57 -0500 Eric Lammerts wrote:
 When extracting, tar sometimes renames a backup back to the original name
 (overwriting the newly-extracted file) for no good reason.
 
 Example:
 # mkdir dir1 dir2
 # echo bla  dir1/file1
 # tar cf test.tar dir1 dir2
 # tar xfv test.tar --backup
 dir1/
 dir1/file1
 Renaming `dir1/file1' to `dir1/file1~'
 dir2/
 Renaming `dir1/file1~' back to `dir1/file1'
 
 This problem is made worse by the fact that if you do the above with tar
 xf instead of tar xfv, tar doesn't print anything at all (and exits
 with status 0)!! Failure to extract a file should be an error, not just a
 message printed only in verbose mode.

Hi Eric,

Thank you for the bug report against the Debian tar package. I've
verified that this bug also exists in tar version 1.22-1.1 as packaged
by Debian.

I've traced through the code a bit and the problem is due to the
handling of directory entries in extract.c:extract_archive(). Here is
what happens when processing dir2 in the above scenario:

1. maybe_backup_file() returns true, but without changing
   after_backup_file (which was set previously to dir1/file1~)

2. extract_dir(), (called through *fun) returns -1 since the
   directory already exists

3. after detecting that an archive member failed to extract, the code
   calls undo_last_backup() to restore the backup. However, at this
   point the file names after_backup_file and before_backup_file still
   correspond to dir1/file1~ and dir1/file1 due to step #1.

So step 1 is definitely a bug, and step 2 might potentially be a bug as
well. I've attached a simple patch to fix the bug in step 1. Sergey,
perhaps you'll have a cleaner fix. Please let me know.

-Carl


From 0465ed84d91583387763ab5ed016d1b58abc1ad2 Mon Sep 17 00:00:00 2001
From: Carl Worth cwo...@cworth.org
Date: Tue, 4 Aug 2009 13:34:52 -0700
Subject: [PATCH] maybe_backup_file: Clear previously-set after_backup_name

Without this, under various conditions, (including file_name being
a directory), maybe_backup_file would return true without having
done anything, nor having set after_backup_name. Then, if any
error was detected while extracting the member, (such as directory
already exists), the previously backed-up file would be restored,
potentially destroying user data.

This closes Debian bug #508199, with thanks to Eric Lammerts for
reporting it with a concise test case.
---
 debian/changelog |3 ++-
 src/misc.c   |4 
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/debian/changelog b/debian/changelog
index 1e26c81..daa0223 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -7,8 +7,9 @@ tar (1.22-1.2) UNRELEASED; urgency=low
 Thanks to Ted T'so for the idea and Sergey Poznyakoff for
 cleaning up my original implementation.
   * Respect DEB_BUILD_OPTIONS=nocheck to conform with Policy 3.8.2
+  * Avoid incorrectly restoring old backup file, closes: #508199
 
- -- Carl Worth cwo...@cworth.org  Tue, 04 Aug 2009 12:07:06 -0700
+ -- Carl Worth cwo...@cworth.org  Tue, 04 Aug 2009 13:36:56 -0700
 
 tar (1.22-1.1) unstable; urgency=low
 
diff --git a/src/misc.c b/src/misc.c
index 951449e..c400bcd 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -422,6 +422,10 @@ maybe_backup_file (const char *file_name, bool this_is_the_archive)
   if (this_is_the_archive  _remdev (file_name))
 return true;
 
+  /* Ensure that no previously backed-up file remains in case we
+   * return early. */
+  assign_string (after_backup_name, 0);
+
   if (stat (file_name, file_stat))
 {
   if (errno == ENOENT)
-- 
1.6.3.3



signature.asc
Description: This is a digitally signed message part