[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#29961: [PATCH] mv -n: do not overwrite the destination
From: |
Kamil Dudka |
Subject: |
bug#29961: [PATCH] mv -n: do not overwrite the destination |
Date: |
Fri, 05 Jan 2018 17:19:47 +0100 |
On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
> 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.
Thanks for the summary!
> 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.
Do you mean without -n?
I am getting the following error message in that case:
mv: cannot move 'a' to 'b': Is a directory
... which is consistent with the original behavior.
> Paul's also avoids a stat() in the common case
> where the initial renameat2() succeeds.
At the cost of _not_ avoiding the renameat2() call in the most common case.
I think both the solutions are equivalent performance-wise.
> 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.
No problem with that. Anyway, I will go with my conservative (or even the
documentation-only) patch for RHEL-7, which was the trigger of this effort,
because Paul's patch comes with changes of behavior observable beyond the
reported scenario.
Kamil
> 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, 2018/01/05
- bug#29961: [PATCH] mv -n: do not overwrite the destination,
Kamil Dudka <=
- 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