[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#29961: [PATCH] mv -n: do not overwrite the destination
From: |
Pádraig Brady |
Subject: |
bug#29961: [PATCH] mv -n: do not overwrite the destination |
Date: |
Fri, 5 Jan 2018 15:29:55 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
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
- bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n', Kamil Dudka, 2018/01/03
- bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n', Pádraig Brady, 2018/01/03
- bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n', Kamil Dudka, 2018/01/03
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Kamil Dudka, 2018/01/04
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Paul Eggert, 2018/01/04
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Kamil Dudka, 2018/01/04
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Paul Eggert, 2018/01/04
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Kamil Dudka, 2018/01/05
- bug#29961: [PATCH] mv -n: do not overwrite the destination,
Pádraig Brady <=
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Kamil Dudka, 2018/01/05
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Paul Eggert, 2018/01/05
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Bernhard Voelker, 2018/01/06
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Paul Eggert, 2018/01/06
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Pádraig Brady, 2018/01/06
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Paul Eggert, 2018/01/10
- bug#29961: [PATCH] mv -n: do not overwrite the destination, Kamil Dudka, 2018/01/10