[coreutils] cp -r --backup dir1 dir2

Version:

coreutils-6.0 (fixed in 6.4)

How to reproduce?

reproduce this bug using:

1. create directory ./dir1

2. create directory ./dir2/dir1

3. coreutils-6.0/src/cp -r --backup dir1 dir2

Symptom:

Incorrect result.

Using the command sequence like section “How to reproduce”, a wrong result “./dir2/dir1~” is created. So now in the directory “dir2”, there are two directories: dir1 and dir1~.

The correct behavior should be, when backing up directory, if already a directory with the name ‘dir1’ exists, then we should not create a new directory with name “dir1~”, but instead, merge the original dir1 into the ./dir2/dir1. (If it is not a directory but a regular file, they should copy the original file with the name file~).

Is there log messages (default verbosity level) printed?

No. It’s a silent failure where programmers need to debug in the dark.

Root cause:

Source code bug. Developer made a simple mistake: within the condition to check whether to backup the directories,  they set ‘backup_directory’ to true to override the directory check!

they blindly set the bool backup_directories variable  in copy.c to true. And then cause a wrong execution pass to call rename()

Bug & Errlog’s log insertion explained in detail (comments written in blue are by us):

In the copy.c file.

static bool  copy_internal ()

{

 … …

 /* XSTAT is a macro to stat. cp is using stat to determine whether the

    destination file (dst_name) already exists. Here XSTAT will

    return zero as the dst_name=”dir2/dir1” already exists! */

 if (XSTAT (x, dst_name, &dst_sb) != 0){

         … …

        new_dst = true;

  }

 else {

    /* Here, the return value of stat (zero) indicates the

       target file EXISTS! so we need to

       convert the name (if regular file, rename by appending a “~”).*/

               … ...

/* Here is the bug and the fix: backup_directories always set to

   true thus it will override the condition check “S_ISDIR(dst_sb.st_mode)”,  

   which won’t be executed in any circumstances. So as long

   as x->backup_type != no_backups, the if branch below will always succeed!

 

  In the failure case: the execution will reach here with dst_sb pointing to

  “dir2/dir1”. The correct logic should be to detect it’s a directory,

  and not going into the if branch, which subsequently will create a

  “dir2/dir1~”. But because of the bug, the failure execution mistakenly

  entered the true branch of the if condition. */

-   bool backup_directories = true;

-   if (x->backup_type != no_backups

-        && (!S_ISDIR (dst_sb.st_mode) || backup_directories))

+   if (x->backup_type != no_backups           

+        && ! dot_or_dotdot (last_component (src_name))           

+        && (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

                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.  */

/* STREQ is a macro for “strcmp”, whose non-matching return value will be

  logged by Errlog. In this case, tmp_backup=”dir2/dir1~”, src_name=”dir1”,

  so they do not match, thus we will have a valuable error message! */

             if (STREQ (tmp_backup, src_name))

               {    

                 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,

                        quote_n (0, dst_name),

                        quote_n (1, src_name));

                 free (tmp_backup);

                 return false;

               }    

 

          if (rename (dst_name, dst_backup) != 0) {

        ...

         }

          else

      {

           backup_succeeded = true;

       }

 }

...

}

Discussions:

By the way, this bug (a non-effective branch condition tested) can potentially be checked statically:

           bool backup_directories = true;

            if (x->backup_type != no_backups

                && (!S_ISDIR (dst_sb.st_mode) || backup_directories))

Here, backup_directories is always true, and !S_ISDIR (dst_sb.st_mode) is USELESS. This condition statement IS NOT well designed.

Can Errlog insert an error message?

Yes. The failed input check pattern. The non-matching return value of strcmp will take us right to the basic block of the buggy code.