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
