bug#29961: [PATCH] mv -n: do not overwrite the destination

[Kamil Dudka]

On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
> Kamil Dudka wrote:
> > -      if (rename (src_name, dst_name) == 0)
> > +      int flags = 0;
> > +      if (x->interactive == I_ALWAYS_NO)
> > +        /* do not replace DST_NAME if it was created since our last check
> > */ +        flags = RENAME_NOREPLACE;
> 
> By then it's too late, as multiple decisions have been made on the basis of
> stat/lstat calls that are still subject to races.

Do you mean in the case of mv -n?  Which decisions exactly?

> It's better to try the
> renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing
> code only if renameat2 fails with errno != EEXIST.
> 
> Plus, there are some other problems when combining -u and -n.

Sounds like a corner case.  Please consider writing a separate patch for that.

> How about the attached patch instead?

I had difficulties trying to evaluate the patch.  It does not compile
with gcc-7.2.1:

src/copy.c: In function 'copy_internal':
src/copy.c:2340:17: error: 'earlier_file' may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
           if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                   dereference))
                                   ~~~~~~~~~~~~
src/copy.c:2050:14: error: 'return_now' may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
           if (return_now)
              ^

So I added initializers to those two variables but then I saw multiple
test-cases failing because of the patch:

FAIL: tests/mv/hard-link-1
FAIL: tests/mv/mv-special-1
FAIL: tests/mv/part-fail
FAIL: tests/mv/part-hardlink
FAIL: tests/mv/part-rename
FAIL: tests/mv/part-symlink

Kamil