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

[Kamil Dudka]

On Friday, January 5, 2018 2:00:52 AM CET Paul Eggert wrote:
> On 01/04/2018 03:01 AM, Kamil Dudka wrote:
> > 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?
> 
> Mostly mv -n, but I suspect problems also for mv without -n.

My patch changes nothing but the 'mv -n' behavior.  It could hardly break
(or even change behavior of) anything else.  Your patch reworks the logic
of copy_internal(), which itself is very fragile, as you learned from the 
first version of your patch.

> It's all
> the decisions that depend on the result of lstat of dst_name, before
> abandon_move decides whether to skip the rename.

I am only fixing the case where the destination file is created after the 
lstat() call.  In that case, the only result is ENOENT, which is harmless.

> With the patch you
> proposed, mv -n could call lstat and get a failure (with errno ==
> ENOENT), then (after another process creates the file) call renameat2
> with RENAME_NOREPLACE and after this fails (with errno == EEXIST)

Exactly.  That is the only case that my patch intended to fix.

> report an error.

Nope.  It will silently succeed with my patch.  Did you try it?

> mv -n should silently succeed in that case.

I agree.

> > Sounds like a corner case. Please consider writing a separate patch
> > for that.
> 
> OK, that's pretty straightforward so I installed it. Please see the
> first attached patch.
> 
> > I had difficulties trying to evaluate the patch. It does not compile
> 
> That's what I get for sending an untested patch. Sorry. I fixed the bugs
> you mentioned and tested the result. Please see the second attached
> patch, which I have not installed.

Thanks for the fixup!

> There is an interesting behavior change with this second patch.
> Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the
> same file". With the patch, 'mv -n a a' silently succeeds. The coreutils
> documentation allows both behaviors. I doubt whether anyone cares, and
> doing it the new way avoids some syscalls so I left it that way.

If you decide to apply your patch anyway, please document this change
at least in the commit message.  Still my concern is that your patch also 
changes the behavior of 'mv' without '-n', which is neither tested, nor 
documented.

Kamil