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

From: Kamil Dudka <[email-masked]>
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