bug#24714: delete-directory race condition

bug#24714: delete-directory race condition

[Glenn Morris]

Package: emacs
Version: 25.1

On current Debian testing, many tests in package-test.el fail for me with:

    Test package-test-update-listing condition:
    (file-error "Removing old name" "No such file or directory"
    "/tmp/pkg-test-user-dir-27293kBj/gnupg/S.gpg-agent.rstrd")

I believe this is due to a race condition in delete-directory.
Emacs seems to be lacking an equivalent of "rm -rf".

(delete-directory "/tmp/foo")

will fail with "No such file or directory" if a file in /tmp/foo
happen to be deleted by some other process in between the time that
delete-directory calls directory-files and the time it calls delete-file.

bug#24714: delete-directory race condition

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Sun, 16 Oct 2016 22:20:45 -0400
> 
> I believe this is due to a race condition in delete-directory.
> Emacs seems to be lacking an equivalent of "rm -rf".
> 
> (delete-directory "/tmp/foo")
> 
> will fail with "No such file or directory" if a file in /tmp/foo
> happen to be deleted by some other process in between the time that
> delete-directory calls directory-files and the time it calls delete-file.

IMO, delete-directory should simply catch ENOENT errors and ignore
them when it deletes files and subdirectories under the "recursive"
option.  Other errors should signal an error as they do now.

bug#24714: delete-directory race condition

[Glenn Morris]

Eli Zaretskii wrote:

>> will fail with "No such file or directory" if a file in /tmp/foo
>> happen to be deleted by some other process in between the time that
>> delete-directory calls directory-files and the time it calls delete-file.
>
> IMO, delete-directory should simply catch ENOENT errors and ignore
> them when it deletes files and subdirectories under the "recursive"
> option.

I don't think that's enough, since a file could equally well be
_created_ by some other process after delete-directory calls directory-files.
Frankly I don't see how Emacs's delete-directory can work reliably as
currently implemented.

bug#24714: delete-directory race condition

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: 24714@debbugs.gnu.org
> Date: Mon, 17 Oct 2016 11:52:31 -0400
> 
> Eli Zaretskii wrote:
> 
> >> will fail with "No such file or directory" if a file in /tmp/foo
> >> happen to be deleted by some other process in between the time that
> >> delete-directory calls directory-files and the time it calls delete-file.
> >
> > IMO, delete-directory should simply catch ENOENT errors and ignore
> > them when it deletes files and subdirectories under the "recursive"
> > option.
> 
> I don't think that's enough, since a file could equally well be
> _created_ by some other process after delete-directory calls directory-files.

It will solve your use case, with files under /tmp, won't it?  A
partial solution is better than no solution at all, IMO.

> Frankly I don't see how Emacs's delete-directory can work reliably as
> currently implemented.

Patches to reimplement it are also welcome.

bug#24714: delete-directory race condition

[Andreas Schwab]

On Okt 17 2016, Glenn Morris <rgm@gnu.org> wrote:

> I don't think that's enough, since a file could equally well be
> _created_ by some other process after delete-directory calls directory-files.

That is ok.  The operation cannot successfully be completed in this case.

> Frankly I don't see how Emacs's delete-directory can work reliably as
> currently implemented.

A property it shares with any other operation recursing on directories,
in Emacs or elsewhere.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24714: delete-directory race condition

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]

Although I didn't reproduce the bug, I changed delete-directory 
according to Eli's suggestion by installing the attached patch into 
master. Glenn, can you please try it in your environment?


[-- Attachment #2: 0001-delete-directory-no-longer-errors-when-racing.patch --]
[-- Type: application/x-patch, Size: 4740 bytes --]

bug#24714: delete-directory race condition

[Glenn Morris]

Andreas Schwab wrote:

> That is ok.  The operation cannot successfully be completed in this case.
>
>> Frankly I don't see how Emacs's delete-directory can work reliably as
>> currently implemented.
>
> A property it shares with any other operation recursing on directories,
> in Emacs or elsewhere.

That's interesting. I'd assumed there was a "Right Way" to do it, and
that it would be whatever coreutil's rm did. If not then we are back to:

> delete-directory should simply catch ENOENT errors and ignore them
> when it deletes files and subdirectories under the "recursive" option.
> Other errors should signal an error as they do now.

This doesn't seem possible without changes at the C level.
Should there be a standard Lisp error for ENOENT?
Currently report_file_errno only does EEXIST -> file_already_exists.

Or should delete-file get a FORCE argument akin to "rm -f"?

bug#24714: delete-directory race condition

[Glenn Morris]

I haven't tested it, but the files--force implementation looks a little
unaesthetic to me. Did you consider adding a standard condition error in
report_file_errno for ENOENT? There would then be symmetry with the
file-already-exists error for EEXIST.

Also, IMO this is purely a bug fix, and should not be mentioned in NEWS,
the lispref, or the doc.

But thanks for the fix!

bug#24714: delete-directory race condition

[Glenn Morris]

Glenn Morris wrote:

> I haven't tested it, but the files--force implementation looks a little
> unaesthetic to me.

PS will it work in non-English locales?

bug#24714: delete-directory race condition

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Tue, 18 Oct 2016 12:59:26 -0400
> Cc: 24714@debbugs.gnu.org
> 
> Glenn Morris wrote:
> 
> > I haven't tested it, but the files--force implementation looks a little
> > unaesthetic to me.
> 
> PS will it work in non-English locales?

Paul, what about this Glenn's question?  The error strings are
localized, I think.

bug#24714: delete-directory race condition

[Paul Eggert]

Eli Zaretskii wrote:
> Paul, what about this Glenn's question?  The error strings are
> localized, I think.

Yes, his comments are spot on. I have written a fix but want to test it more 
before installing.

bug#24714: delete-directory race condition

[Daniel Colascione]

On 10/19/2016 12:23 AM, Eli Zaretskii wrote:
>> From: Glenn Morris <rgm@gnu.org>
>> Date: Tue, 18 Oct 2016 12:59:26 -0400
>> Cc: 24714@debbugs.gnu.org
>>
>> Glenn Morris wrote:
>>
>>> I haven't tested it, but the files--force implementation looks a little
>>> unaesthetic to me.
>>
>> PS will it work in non-English locales?
>
> Paul, what about this Glenn's question?  The error strings are
> localized, I think.

What about attaching the numeric errno value to the error message as a 
text property in report_file_errno?

bug#24714: delete-directory race condition

[Andreas Schwab]

On Okt 21 2016, Daniel Colascione <dancol@dancol.org> wrote:

> What about attaching the numeric errno value to the error message as a
> text property in report_file_errno?

errno values aren't portable either.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24714: delete-directory race condition

[Daniel Colascione]

On 10/21/2016 05:09 AM, Andreas Schwab wrote:
> On Okt 21 2016, Daniel Colascione <dancol@dancol.org> wrote:
>
>> What about attaching the numeric errno value to the error message as a
>> text property in report_file_errno?
>
> errno values aren't portable either.

True, but you can provide the value of EEXIST and others as constants 
accessible to lisp.

bug#24714: delete-directory race condition

[Eli Zaretskii]

> Cc: eggert@cs.ucla.edu, 24714@debbugs.gnu.org
> From: Daniel Colascione <dancol@dancol.org>
> Date: Fri, 21 Oct 2016 04:53:40 -0700
> 
> > Paul, what about this Glenn's question?  The error strings are
> > localized, I think.
> 
> What about attaching the numeric errno value to the error message as a 
> text property in report_file_errno?

IMO, Glenn suggested a cleaner solution: to have a symbol for ENOENT.

bug#24714: delete-directory race condition

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On 10/18/2016 09:55 AM, Glenn Morris wrote:
> Did you consider adding a standard condition error in
> report_file_errno for ENOENT? There would then be symmetry with the
> file-already-exists error for EEXIST.

Again, thanks for pointing that out. I installed the attached patch, 
which does that. This also should fix the locale problem you mentioned 
later.


[-- Attachment #2: 0001-New-error-file-missing.patch --]
[-- Type: application/x-patch, Size: 16007 bytes --]