GNU bug report logs - #29961
[PATCH] mv: document the missing atomicity of 'mv -n'

Previous Next

Package: coreutils;

Reported by: Kamil Dudka <kdudka <at> redhat.com>

Date: Wed, 3 Jan 2018 15:03:02 UTC

Severity: normal

Tags: patch

Done: Paul Eggert <eggert <at> cs.ucla.edu>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29961 in the body.
You can then email your comments to 29961 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 03 Jan 2018 15:03:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Kamil Dudka <kdudka <at> redhat.com>:
New bug report received and forwarded. Copy sent to bug-coreutils <at> gnu.org. (Wed, 03 Jan 2018 15:03:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] mv: document the missing atomicity of 'mv -n'
Date: Wed,  3 Jan 2018 16:01:47 +0100
* src/mv.c (usage): Add note about the missing atomicity of 'mv -n'.
* doc/coreutils.texi (mv invocation): Likewise.
---
 doc/coreutils.texi | 3 +++
 src/mv.c           | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3fa083085..e7ca6a737 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9660,6 +9660,9 @@ If the response is not affirmative, the file is skipped.
 Do not overwrite an existing file.
 @mvOptsIfn
 This option is mutually exclusive with @option{-b} or @option{--backup} option.
+Note that the operation is not atomic.  If @var{dest} appears after
+@command{mv} has checked its non-existence, it can be overwritten despite
+the @option{-n} option is used.
 
 @item -u
 @itemx --update
diff --git a/src/mv.c b/src/mv.c
index a8df730a7..a08e75649 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -306,6 +306,8 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
   -i, --interactive            prompt before overwrite\n\
   -n, --no-clobber             do not overwrite an existing file\n\
 If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
+NOTE: The operation is not atomic.  If DEST appears after mv has checked its\n\
+non-existence, it can be overwritten despite the -n option is used.\n\
 "), stdout);
       fputs (_("\
       --strip-trailing-slashes  remove any trailing slashes from each SOURCE\n\
-- 
2.13.6





Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 03 Jan 2018 15:09:01 GMT) Full text and rfc822 format available.

Message #8 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Kamil Dudka <kdudka <at> redhat.com>, 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n'
Date: Wed, 3 Jan 2018 15:08:51 +0000
On 03/01/18 15:01, Kamil Dudka wrote:
> * src/mv.c (usage): Add note about the missing atomicity of 'mv -n'.
> * doc/coreutils.texi (mv invocation): Likewise.
> ---
>  doc/coreutils.texi | 3 +++
>  src/mv.c           | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 3fa083085..e7ca6a737 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -9660,6 +9660,9 @@ If the response is not affirmative, the file is skipped.
>  Do not overwrite an existing file.
>  @mvOptsIfn
>  This option is mutually exclusive with @option{-b} or @option{--backup} option.
> +Note that the operation is not atomic.  If @var{dest} appears after
> +@command{mv} has checked its non-existence, it can be overwritten despite
> +the @option{-n} option is used.
>  
>  @item -u
>  @itemx --update
> diff --git a/src/mv.c b/src/mv.c
> index a8df730a7..a08e75649 100644
> --- a/src/mv.c
> +++ b/src/mv.c
> @@ -306,6 +306,8 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
>    -i, --interactive            prompt before overwrite\n\
>    -n, --no-clobber             do not overwrite an existing file\n\
>  If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
> +NOTE: The operation is not atomic.  If DEST appears after mv has checked its\n\
> +non-existence, it can be overwritten despite the -n option is used.\n\
>  "), stdout);
>        fputs (_("\
>        --strip-trailing-slashes  remove any trailing slashes from each SOURCE\n\

Eep, Seems like we should use RENAME_NOREPLACE in this case,
rather than document the caveat?
This is already used in shred.

cheers,
Pádraig




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 03 Jan 2018 16:27:02 GMT) Full text and rfc822 format available.

Message #11 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 29961 <at> debbugs.gnu.org, bug-coreutils <at> gnu.org
Subject: Re: bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n'
Date: Wed, 03 Jan 2018 17:27:14 +0100
On Wednesday, January 3, 2018 4:08:51 PM CET Pádraig Brady wrote:
> Eep, Seems like we should use RENAME_NOREPLACE in this case,
> rather than document the caveat?

Thanks for the suggestion!  I will give it a try...

Kamil

> This is already used in shred.
> 
> cheers,
> Pádraig




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 03 Jan 2018 16:28:01 GMT) Full text and rfc822 format available.

Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 03 Jan 2018 18:25:02 GMT) Full text and rfc822 format available.

Message #17 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Kamil Dudka <kdudka <at> redhat.com>
Cc: 29961 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#29961: [PATCH] mv: document the missing atomicity of 'mv -n'
Date: Wed, 3 Jan 2018 10:23:56 -0800
On Wed, Jan 3, 2018 at 8:27 AM, Kamil Dudka <kdudka <at> redhat.com> wrote:
> On Wednesday, January 3, 2018 4:08:51 PM CET Pádraig Brady wrote:
>> Eep, Seems like we should use RENAME_NOREPLACE in this case,
>> rather than document the caveat?

Good catch/fix. Thanks to both of you.




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Thu, 04 Jan 2018 09:01:02 GMT) Full text and rfc822 format available.

Message #20 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 29961 <at> debbugs.gnu.org
Subject: [PATCH] mv -n: do not overwrite the destination
Date: Thu,  4 Jan 2018 10:00:03 +0100
... if it is created by another process after mv has checked its
non-existence.

* src/copy.c (copy_internal): Use renameat2 (..., RENAME_NOREPLACE)
if called by mv -n.  If it fails with EEXIST in that case, pretend
successful rename as if the existing destination file was detected
by the preceding lstat call.
* NEWS: Mention the fix.
Fixes https://bugs.gnu.org/29961
---
 NEWS       |  3 +++
 src/copy.c | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index b89254f7e..e463ce6db 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   df no longer hangs when given a fifo argument.
   [bug introduced in coreutils-7.3]
 
+  mv -n no longer overwrites the destination if it is created by another
+  process after mv has checked its non-existence.
+
   ptx -S no longer infloops for a pattern which returns zero-length matches.
   [the bug dates back to the initial implementation]
 
diff --git a/src/copy.c b/src/copy.c
index 2a804945e..be4e357a8 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -53,6 +53,7 @@
 #include "ignore-value.h"
 #include "ioblksize.h"
 #include "quote.h"
+#include "renameat2.h"
 #include "root-uid.h"
 #include "same.h"
 #include "savedir.h"
@@ -2319,7 +2320,12 @@ copy_internal (char const *src_name, char const *dst_name,
 
   if (x->move_mode)
     {
-      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;
+
+      if (renameat2 (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags) == 0)
         {
           if (x->verbose)
             {
@@ -2351,6 +2357,15 @@ copy_internal (char const *src_name, char const *dst_name,
           return true;
         }
 
+      if ((flags & RENAME_NOREPLACE) && (errno == EEXIST))
+        {
+          /* Pretend the rename succeeded, so the caller (mv)
+             doesn't end up removing the source file.  */
+          if (rename_succeeded)
+            *rename_succeeded = true;
+          return true;
+        }
+
       /* FIXME: someday, consider what to do when moving a directory into
          itself but when source and destination are on different devices.  */
 
-- 
2.13.6





Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Thu, 04 Jan 2018 09:50:01 GMT) Full text and rfc822 format available.

Message #23 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Kamil Dudka <kdudka <at> redhat.com>, Pádraig Brady
 <P <at> draigBrady.com>
Cc: 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Thu, 4 Jan 2018 01:48:56 -0800
[Message part 1 (text/plain, inline)]
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. It's better to try the 
renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing code 
only if renameat2 fails with errno != EEXIST.

Plus, there are some other problems when combining -u and -n.

How about the attached patch instead?
[0001-mv-improve-n-atomicity.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Thu, 04 Jan 2018 11:02:01 GMT) Full text and rfc822 format available.

Message #26 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 29961 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Thu, 04 Jan 2018 12:01:28 +0100
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?

> It's better to try the
> renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing
> code only if renameat2 fails with errno != EEXIST.
> 
> Plus, there are some other problems when combining -u and -n.

Sounds like a corner case.  Please consider writing a separate patch for that.

> How about the attached patch instead?

I had difficulties trying to evaluate the patch.  It does not compile
with gcc-7.2.1:

src/copy.c: In function 'copy_internal':
src/copy.c:2340:17: error: 'earlier_file' may be used uninitialized in this function [-Werror=maybe-uninitialized]
           if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                   dereference))
                                   ~~~~~~~~~~~~
src/copy.c:2050:14: error: 'return_now' may be used uninitialized in this function [-Werror=maybe-uninitialized]
           if (return_now)
              ^

So I added initializers to those two variables but then I saw multiple
test-cases failing because of the patch:

FAIL: tests/mv/hard-link-1
FAIL: tests/mv/mv-special-1
FAIL: tests/mv/part-fail
FAIL: tests/mv/part-hardlink
FAIL: tests/mv/part-rename
FAIL: tests/mv/part-symlink

Kamil




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Fri, 05 Jan 2018 01:02:02 GMT) Full text and rfc822 format available.

Message #29 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Kamil Dudka <kdudka <at> redhat.com>
Cc: 29961 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Thu, 4 Jan 2018 17:00:52 -0800
[Message part 1 (text/plain, inline)]
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.  It's all 
the decisions that depend on the result of lstat of dst_name, before 
abandon_move decides whether to skip the rename. 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) report 
an error. mv -n should silently succeed in that case.

> 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.

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.
[0001-mv-n-overrides-u.patch (text/x-patch, attachment)]
[0002-mv-improve-n-atomicity.patch (text/x-patch, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Fri, 05 Jan 2018 11:56:02 GMT) Full text and rfc822 format available.

Message #32 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 29961 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Fri, 05 Jan 2018 12:56:17 +0100
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




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Fri, 05 Jan 2018 15:30:02 GMT) Full text and rfc822 format available.

Message #35 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Kamil Dudka <kdudka <at> redhat.com>, Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Fri, 5 Jan 2018 15:29:55 +0000
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




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Fri, 05 Jan 2018 16:20:01 GMT) Full text and rfc822 format available.

Message #38 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 29961 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: 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




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Fri, 05 Jan 2018 23:00:02 GMT) Full text and rfc822 format available.

Message #41 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Kamil Dudka <kdudka <at> redhat.com>, Pádraig Brady
 <P <at> draigbrady.com>
Cc: 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Fri, 5 Jan 2018 14:59:06 -0800
[Message part 1 (text/plain, inline)]
On 01/05/2018 08:19 AM, Kamil Dudka wrote:

> 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.
>

Ah, you're right. Sorry, I misread your patch. It should work.

> On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
>
>> 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 expect that the most common case is 'mv A B' where B does not already 
exist. This is the case that avoids the stat of B.

Come to think of it, we don't need to stat A either, when the initial 
renameat2 succeeds. Attached is a revised proposed patchset to do that. 
The first is the same as before; the second causes 'mv A B' to issue 
just a renameat2 syscall (with no calls to stat) in the common case 
where A exists and B does not.

> 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
Sounds good.
[0001-mv-improve-n-atomicity.txt (text/plain, attachment)]
[0002-mv-fewer-syscalls-for-mv-a-b.txt (text/plain, attachment)]
[0003-mv-clarify-mv-n-A-A-change.txt (text/plain, attachment)]

Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Sat, 06 Jan 2018 09:23:01 GMT) Full text and rfc822 format available.

Message #44 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Kamil Dudka <kdudka <at> redhat.com>,
 Pádraig Brady <P <at> draigbrady.com>
Cc: 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Sat, 6 Jan 2018 10:22:05 +0100
On 01/05/2018 11:59 PM, Paul Eggert wrote:
> Attached is a revised proposed patchset to do that.

I didn't have a look at the details, but - as we're changing the
behavior for some cases - it would be good to also add some tests.

Have a nice day,
Berny




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Sat, 06 Jan 2018 10:07:02 GMT) Full text and rfc822 format available.

Message #47 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Kamil Dudka <kdudka <at> redhat.com>
Cc: 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Sat, 6 Jan 2018 10:06:04 +0000
On 05/01/18 22:59, Paul Eggert wrote:
> On 01/05/2018 08:19 AM, Kamil Dudka wrote:
> 
>> 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.
>>
> 
> Ah, you're right. Sorry, I misread your patch. It should work.
> 
>> On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
>>
>>> 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 expect that the most common case is 'mv A B' where B does not already 
> exist. This is the case that avoids the stat of B.
> 
> Come to think of it, we don't need to stat A either, when the initial 
> renameat2 succeeds. Attached is a revised proposed patchset to do that. 
> The first is the same as before; the second causes 'mv A B' to issue 
> just a renameat2 syscall (with no calls to stat) in the common case 
> where A exists and B does not.

This second patch looks good also.

thanks,
Pádraig.





Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Sat, 06 Jan 2018 20:11:01 GMT) Full text and rfc822 format available.

Message #50 received at 29961 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Kamil Dudka <kdudka <at> redhat.com>, Pádraig Brady
 <P <at> draigbrady.com>
Cc: 29961 <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Sat, 6 Jan 2018 12:10:01 -0800
Bernhard Voelker wrote:
> as we're changing the
> behavior for some cases - it would be good to also add some tests

That would make sense if the edge-case behavior was documented and the change is 
something that users could rely on. But the behavior is not documented, and I'm 
not sure we want to promise that the behavior won't change back. (Maybe I'm 
being too cautious?)




Reply sent to Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility. (Wed, 10 Jan 2018 08:54:02 GMT) Full text and rfc822 format available.

Notification sent to Kamil Dudka <kdudka <at> redhat.com>:
bug acknowledged by developer. (Wed, 10 Jan 2018 08:54:02 GMT) Full text and rfc822 format available.

Message #55 received at 29961-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Kamil Dudka <kdudka <at> redhat.com>
Cc: 29961-done <at> debbugs.gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Wed, 10 Jan 2018 00:53:07 -0800
No further comment, so I installed the proposed patches and I'm closing this bug 
report.




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 10 Jan 2018 10:48:01 GMT) Full text and rfc822 format available.

Message #58 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 29961-done <at> debbugs.gnu.org,
 Pádraig Brady <P <at> draigbrady.com>, bug-coreutils <at> gnu.org
Subject: Re: bug#29961: [PATCH] mv -n: do not overwrite the destination
Date: Wed, 10 Jan 2018 11:47:55 +0100
On Wednesday, January 10, 2018 9:53:07 AM CET Paul Eggert wrote:
> No further comment, so I installed the proposed patches and I'm closing this
> bug report.

Sorry.  There were just too many changes in your patches for me to review
them quickly enough (and most of the changes were unrelated to my initial 
submission anyway).

Thank you for fixing it!

Kamil




Information forwarded to bug-coreutils <at> gnu.org:
bug#29961; Package coreutils. (Wed, 10 Jan 2018 10:48:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 07 Feb 2018 12:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 6 years and 52 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.