Kamil Dudka wrote:
If the user explicitly asks for a simple backup, cp should either make a single backup or fail honestly if the simple backup cannot be made.
Fair enough. I installed the attached additional patch to do that. Although this reintroduces a race, I guess we've lived with the race for quite some time.
From 929501623820b8d2e4995542ededf2eebbb32894 Mon Sep 17 00:00:00 2001 From: Paul Eggert <[email protected]> Date: Tue, 1 Aug 2017 13:14:03 -0700 Subject: [PATCH] copy: go back to failing 'cp --backup a~ a' Suggested by Kamil Dudka in: http://lists.gnu.org/archive/html/coreutils/2017-07/msg00072.html * NEWS: Document the changed nature of the fix. * doc/coreutils.texi, tests/cp/backup-is-src.sh: * tests/mv/backup-is-src.sh: Revert previous change. * src/copy.c (source_is_dst_backup): New function. (copy_internal): Use it. Fail instead of falling back on numbered backups when it looks like the backup will overwrite the source. Although this reintroduces a race, it's more compatible with previous behavior. --- NEWS | 6 +++--- doc/coreutils.texi | 18 ++++++++---------- src/copy.c | 48 +++++++++++++++++++++++++++++++++++------------ tests/cp/backup-is-src.sh | 16 +++++++++------- tests/mv/backup-is-src.sh | 23 +++++++++++++++-------- 5 files changed, 71 insertions(+), 40 deletions(-) diff --git a/NEWS b/NEWS index 13bbc96..eb0c0dc 100644 --- a/NEWS +++ b/NEWS @@ -15,10 +15,10 @@ GNU coreutils NEWS -*- outline -*- 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. + cp, install, ln, and mv no longer lose data when asked to copy a + backup file to its original via a differently-spelled file name. 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. + now fails instead of losing the data. [the bug dates back to the initial implementation] cp, install, ln, and mv now ignore nonsensical backup suffixes. diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 8c4746e..77e993e 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -865,19 +865,17 @@ Never make backups. @opindex numbered @r{backup method} Always make numbered backups. -@item simple -@itemx never -@opindex simple @r{backup method} -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. +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}. @end table diff --git a/src/copy.c b/src/copy.c index 263abc1..9a30e16 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1806,6 +1806,29 @@ should_dereference (const struct cp_options *x, bool command_line_arg) && command_line_arg); } +/* Return true if the source file with basename SRCBASE and status SRC_ST + is likely to be the simple backup file for DST_NAME. */ +static bool +source_is_dst_backup (char const *srcbase, struct stat const *src_st, + char const *dst_name) +{ + size_t srcbaselen = strlen (srcbase); + char const *dstbase = last_component (dst_name); + size_t dstbaselen = strlen (dstbase); + size_t suffixlen = strlen (simple_backup_suffix); + if (! (srcbaselen == dstbaselen + suffixlen + && memcmp (srcbase, dstbase, dstbaselen) == 0 + && STREQ (srcbase + dstbaselen, simple_backup_suffix))) + return false; + size_t dstlen = strlen (dst_name); + char *dst_back = xmalloc (dstlen + suffixlen + 1); + strcpy (mempcpy (dst_back, dst_name, dstlen), simple_backup_suffix); + struct stat dst_back_sb; + int dst_back_status = stat (dst_back, &dst_back_sb); + free (dst_back); + return dst_back_status == 0 && SAME_INODE (*src_st, dst_back_sb); +} + /* Copy the file SRC_NAME to the file DST_NAME. The files may be of any type. NEW_DST should be true if the file DST_NAME cannot exist because its parent directory was just created; NEW_DST should @@ -2081,23 +2104,24 @@ copy_internal (char const *src_name, char const *dst_name, existing hierarchy. */ && (x->move_mode || ! S_ISDIR (dst_sb.st_mode))) { - /* Silently use numbered backups if creating the backup - file might destroy the source file. Otherwise, the commands: + /* Fail if creating the backup file would likely 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~. */ - enum backup_type backup_type = x->backup_type; - if (backup_type != numbered_backups) + if (x->backup_type != numbered_backups + && source_is_dst_backup (srcbase, &src_sb, dst_name)) { - 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; + 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)); + return false; } - char *tmp_backup = backup_file_rename (dst_name, backup_type); + char *tmp_backup = backup_file_rename (dst_name, x->backup_type); /* FIXME: use fts: Using alloca for a file name that may be arbitrarily diff --git a/tests/cp/backup-is-src.sh b/tests/cp/backup-is-src.sh index 8077106..3e4a79f 100755 --- a/tests/cp/backup-is-src.sh +++ b/tests/cp/backup-is-src.sh @@ -20,15 +20,17 @@ 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 not trash the source. -cp --b=simple a~ ./a > out 2>&1 || fail=1 +# This cp command should exit nonzero. +cp --b=simple a~ a > out 2>&1 && fail=1 -compare a~0 a || fail=1 -compare a~0 a~ || fail=1 -compare a0 a.~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 Exit $fail diff --git a/tests/mv/backup-is-src.sh b/tests/mv/backup-is-src.sh index c1310de..04e9f7c 100755 --- a/tests/mv/backup-is-src.sh +++ b/tests/mv/backup-is-src.sh @@ -22,18 +22,25 @@ 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" a20 || framework_failure_ +rm -f "$a" "$a2" || 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 not trash the source. -mv --b=simple "$a2" "$a" > out 2>&1 || fail=1 +# This mv command should exit nonzero. +mv --b=simple "$a2" "$a" > out 2>&1 && fail=1 -compare a20 "$a" || fail=1 -compare a0 "$a.~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 Exit $fail -- 2.7.4
