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

[Pádraig Brady]

On 05/01/18 11:56, Kamil Dudka wrote:
> 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.

Thanks to both of you.
The approaches can be summarized as:

Orig:
---------------------------------
stat() => ENOENT
<file created externally>
rename()  may clobber file

Kamil's:
---------------------------------
stat() => ENOENT
<file created externally>
renameat()  doesn't clobber file if -n
exit early if -n && errno==EEXIST

Paul's:
---------------------------------
renameat2() => EEXIST
 -n => exit early
if (renameat2_failed)
 unless EEXIST && -n
  stat()
if (renameat2_failed)
 rename()

I think both patches ensure rename() doesn't clobber when -n is specified.

Paul's is more encompassing for the non -n case.
For example if a directory was _created_ externally
after the stat() in Kamil's logic above,
it could be erroneously clobbered.
Paul's also avoids a stat() in the common case
where the initial renameat2() succeeds.

Both versions are still susceptible to erroneous clobbering
if the destination file was externally _replaced_
by a directory for example, after the stat().
Though that is less likely.

Paul's fix looks good to apply here,
since it's more encompassing.

cheers,
Pádraig