[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 3/4] copy: make backup files more reliably
|
From: |
Paul Eggert |
|
Subject: |
[PATCH 3/4] copy: make backup files more reliably |
|
Date: |
Sun, 30 Jul 2017 17:19:06 -0700 |
* NEWS, doc/coreutils.texi (Backup options): Document the change.
* bootstrap.conf (gnulib_modules): Add backup-rename.
* src/copy.c (copy_internal): Silently switch to numbered backups
if a simple backup might lose data. Use backup_file_rename
to avoid races with numbered backups.
* tests/cp/backup-is-src.sh, tests/mv/backup-is-src.sh:
Adjust to match new behavior.
---
NEWS | 12 +++++++++
bootstrap.conf | 1 +
doc/coreutils.texi | 18 +++++++------
src/copy.c | 67 +++++++++++++++++------------------------------
tests/cp/backup-is-src.sh | 16 +++++------
tests/mv/backup-is-src.sh | 23 ++++++----------
6 files changed, 62 insertions(+), 75 deletions(-)
diff --git a/NEWS b/NEWS
index 110229b..7c22ebb 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,18 @@ GNU coreutils NEWS -*-
outline -*-
mv would leave such symlinks behind in the source file system.
[the bug dates back to the initial implementation]
+ When creating numbered backups, cp, install, ln, and mv now avoid
+ races that could lose backup data in unlikely circumstances. Since
+ the fix relies on the renameat2 system call of Linux kernel 3.15 and
+ later, the races are still present on other platforms.
+ [the bug dates back to the initial implementation]
+
+ cp, install, ln, and mv now use a numbered instead of a simple
+ backup if copying a backup file to what might be its original.
+ E.g., 'rm -f a a~; : > a; echo data > a~; cp --backup=simple a~ ./a'
+ now makes a numbered backup file instead of losing the data.
+ [the bug dates back to the initial implementation]
+
date and touch no longer overwrite the heap with large
user specified TZ values (CVE-2017-7476).
[bug introduced in coreutils-8.27]
diff --git a/bootstrap.conf b/bootstrap.conf
index 188b1ef..f8f65f7 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -35,6 +35,7 @@ gnulib_modules="
assert
autobuild
backupfile
+ backup-rename
base32
base64
buffer-lcm
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 77e993e..8c4746e 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -865,17 +865,19 @@ Never make backups.
@opindex numbered @r{backup method}
Always make numbered backups.
-@item existing
-@itemx nil
-@opindex existing @r{backup method}
-Make numbered backups of files that already have them, simple backups
-of the others.
-
@item simple
@itemx never
@opindex simple @r{backup method}
-Always make simple backups. Please note @samp{never} is not to be
-confused with @samp{none}.
+Make simple backups, except make a numbered backup if the source might
+be a simple backup of the destination; this avoids losing source data.
+Please note @samp{never} is not to be confused with @samp{none}.
+
+@item existing
+@itemx nil
+@opindex existing @r{backup method}
+Make numbered backups of files that already have them, or if the source
+might be a simple backup of the destination.
+Otherwise, make simple backups.
@end table
diff --git a/src/copy.c b/src/copy.c
index a2eee1b..263abc1 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1837,7 +1837,6 @@ copy_internal (char const *src_name, char const *dst_name,
bool restore_dst_mode = false;
char *earlier_file = NULL;
char *dst_backup = NULL;
- bool backup_succeeded = false;
bool delayed_ok;
bool copied_as_regular = false;
bool dest_is_symlink = false;
@@ -2070,10 +2069,11 @@ copy_internal (char const *src_name, char const
*dst_name,
}
}
+ char const *srcbase;
if (x->backup_type != no_backups
/* Don't try to back up a destination if the last
component of src_name is "." or "..". */
- && ! dot_or_dotdot (last_component (src_name))
+ && ! dot_or_dotdot (srcbase = last_component (src_name))
/* Create a backup of each destination directory in move mode,
but not in copy mode. FIXME: it might make sense to add an
option to suppress backup creation also for move mode.
@@ -2081,55 +2081,38 @@ copy_internal (char const *src_name, char const
*dst_name,
existing hierarchy. */
&& (x->move_mode || ! S_ISDIR (dst_sb.st_mode)))
{
- char *tmp_backup = find_backup_file_name (dst_name,
- x->backup_type);
-
- /* Detect (and fail) when creating the backup file would
- destroy the source file. Before, running the commands
+ /* Silently use numbered backups if creating the backup
+ file might destroy the source file. Otherwise, the commands:
cd /tmp; rm -f a a~; : > a; echo A > a~; cp --b=simple a~ a
would leave two zero-length files: a and a~. */
- /* FIXME: but simply change e.g., the final a~ to './a~'
- and the source will still be destroyed. */
- if (STREQ (tmp_backup, src_name))
+ enum backup_type backup_type = x->backup_type;
+ if (backup_type != numbered_backups)
{
- const char *fmt;
- fmt = (x->move_mode
- ? _("backing up %s would destroy source; %s not moved")
- : _("backing up %s would destroy source; %s not copied"));
- error (0, 0, fmt,
- quoteaf_n (0, dst_name),
- quoteaf_n (1, src_name));
- free (tmp_backup);
- return false;
+ size_t srcbaselen = strlen (srcbase);
+ char const *dstbase = last_component (dst_name);
+ size_t dstbaselen = strlen (dstbase);
+ if (srcbaselen == dstbaselen + strlen (simple_backup_suffix)
+ && memcmp (srcbase, dstbase, dstbaselen) == 0
+ && STREQ (srcbase + dstbaselen, simple_backup_suffix))
+ backup_type = numbered_backups;
}
+ char *tmp_backup = backup_file_rename (dst_name, backup_type);
+
/* FIXME: use fts:
Using alloca for a file name that may be arbitrarily
long is not recommended. In fact, even forming such a name
should be discouraged. Eventually, this code will be
rewritten
to use fts, so using alloca here will be less of a problem.
*/
- ASSIGN_STRDUPA (dst_backup, tmp_backup);
- free (tmp_backup);
- /* In move mode, when src_name and dst_name are on the
- same partition (FIXME, and when they are non-directories),
- make the operation atomic: link dest
- to backup, then rename src to dest. */
- if (rename (dst_name, dst_backup) != 0)
+ if (tmp_backup)
{
- if (errno != ENOENT)
- {
- error (0, errno, _("cannot backup %s"),
- quoteaf (dst_name));
- return false;
- }
- else
- {
- dst_backup = NULL;
- }
+ ASSIGN_STRDUPA (dst_backup, tmp_backup);
+ free (tmp_backup);
}
- else
+ else if (errno != ENOENT)
{
- backup_succeeded = true;
+ error (0, errno, _("cannot backup %s"), quoteaf (dst_name));
+ return false;
}
new_dst = true;
}
@@ -2194,7 +2177,7 @@ copy_internal (char const *src_name, char const *dst_name,
sure we'll create a directory. Also don't announce yet when moving
so we can distinguish renames versus copies. */
if (x->verbose && !x->move_mode && !S_ISDIR (src_mode))
- emit_verbose (src_name, dst_name, backup_succeeded ? dst_backup : NULL);
+ emit_verbose (src_name, dst_name, dst_backup);
/* Associate the destination file name with the source device and inode
so that if we encounter a matching dev/ino pair in the source tree
@@ -2317,8 +2300,7 @@ copy_internal (char const *src_name, char const *dst_name,
if (x->verbose)
{
printf (_("renamed "));
- emit_verbose (src_name, dst_name,
- backup_succeeded ? dst_backup : NULL);
+ emit_verbose (src_name, dst_name, dst_backup);
}
if (x->set_security_context)
@@ -2423,8 +2405,7 @@ copy_internal (char const *src_name, char const *dst_name,
if (x->verbose && !S_ISDIR (src_mode))
{
printf (_("copied "));
- emit_verbose (src_name, dst_name,
- backup_succeeded ? dst_backup : NULL);
+ emit_verbose (src_name, dst_name, dst_backup);
}
new_dst = true;
}
diff --git a/tests/cp/backup-is-src.sh b/tests/cp/backup-is-src.sh
index 3e4a79f..8077106 100755
--- a/tests/cp/backup-is-src.sh
+++ b/tests/cp/backup-is-src.sh
@@ -20,17 +20,15 @@
print_ver_ cp
echo a > a || framework_failure_
+cp a a0 || framework_failure_
echo a-tilde > a~ || framework_failure_
+cp a~ a~0 || framework_failure_
-# This cp command should exit nonzero.
-cp --b=simple a~ a > out 2>&1 && fail=1
+# This cp command should not trash the source.
+cp --b=simple a~ ./a > out 2>&1 || fail=1
-sed "s,cp:,XXX:," out > out2
-
-cat > exp <<\EOF
-XXX: backing up 'a' would destroy source; 'a~' not copied
-EOF
-
-compare exp out2 || fail=1
+compare a~0 a || fail=1
+compare a~0 a~ || fail=1
+compare a0 a.~1~ || fail=1
Exit $fail
diff --git a/tests/mv/backup-is-src.sh b/tests/mv/backup-is-src.sh
index 04e9f7c..c1310de 100755
--- a/tests/mv/backup-is-src.sh
+++ b/tests/mv/backup-is-src.sh
@@ -22,25 +22,18 @@ cleanup_() { rm -rf "$other_partition_tmpdir"; }
. "$abs_srcdir/tests/other-fs-tmpdir"
a="$other_partition_tmpdir/a"
-a2="$other_partition_tmpdir/a~"
+a2="$other_partition_tmpdir/./a~"
-rm -f "$a" "$a2" || framework_failure_
+rm -f "$a" "$a2" a20 || framework_failure_
echo a > "$a" || framework_failure_
+cp "$a" a0 || framework_failure_
echo a2 > "$a2" || framework_failure_
+cp "$a2" a20 || framework_failure_
-# This mv command should exit nonzero.
-mv --b=simple "$a2" "$a" > out 2>&1 && fail=1
+# This mv command should not trash the source.
+mv --b=simple "$a2" "$a" > out 2>&1 || fail=1
-sed \
- -e "s,mv:,XXX:," \
- -e "s,$a,YYY," \
- -e "s,$a2,ZZZ," \
- out > out2
-
-cat > exp <<\EOF
-XXX: backing up 'YYY' would destroy source; 'ZZZ' not moved
-EOF
-
-compare exp out2 || fail=1
+compare a20 "$a" || fail=1
+compare a0 "$a.~1~" || fail=1
Exit $fail
--
2.7.4