From: Paul Eggert Date: Mon, 31 Jul 2017 00:11:24 +0000 (-0700) Subject: copy: make backup files more reliably X-Git-Tag: v8.28~50 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0d74ac470fb99701510eecb274e607ba83d78c3f;p=thirdparty%2Fcoreutils.git copy: make backup files more reliably * 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. --- diff --git a/NEWS b/NEWS index 110229bd8e..7c22ebbb42 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 188b1ef786..f8f65f746f 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 77e993e467..8c4746eefb 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 a2eee1be52..263abc1e63 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 3e4a79f397..8077106459 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 04e9f7c813..c1310de1a5 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