Re: rm -r sometimes produces errors under NFS

[Jim Meyering]

Paul Eggert <address@hidden> wrote:

> Regardless of the NFS issue, if euidaccess discovers trouble with a
> directory entry (i.e., euidaccess fails for reasons other than
> EACCES), then it would make sense for 'rm' to issue a diagnostic
> rather than ignoring the problem and going ahead with 'unlink'.

Sounds good, and I've applied it, but I confess I'm leery of skipping
the unlink based on a potentially incorrect result from euidaccess.
The scenario in which a would-have-succeeded unlink is now skipped
is contrived: ((uid!=euid || a very long file name) && an ACL granting
write access that is denied by permission bits) so few will notice.

To implement this part of rm properly we need real support for eaccess
(or euidaccess) and eaccessat functions (in the uid!=euid case,
glibc's euidaccess looks only at permission bits, not ACLs or xattrs)
implementation that's in glibc.  Back when I lamented that Solaris
was doing it right with their openat support, but with Linux we had to
resort to the less-efficient emulation, Ulrich did a lot of the work to
get openat and all the other *at functions into glibc and the kernel,
so maybe he can help here, too.

There's even a comment in glibc's sysdeps/posix/euidaccess.c:
  /* XXX Add support for ACLs.  */
so GNU rm may not be the only application interested in such
an addition.

BTW, glibc already has faccessat, but that's for actual IDs, not the
effective ones required to implement the POSIX-mandated semantics of
rm, when it prompts.

> For Vincent's case, this would cause 'rm' to issue a diagnostic like
> "rm: cannot remove `dir/file:': Stale NFS file handle" which is more
> accurate than the somewhat confusing "no such file or directory"
> diagnostic that 'rm' currently generates.
>
> Here's a more-diabolical example.  You invoke 'rm a/b/c', and a/b/c is
> read-only.  Some other process temporarily renames a/b to a/B between
> the time that 'rm' stats a/b/c and the time 'rm' invokes
> euidaccess("a/b/c", W_OK); the other process then renames a/B back to
> a/b after euidaccess is invoked.  In this case, euidaccess fails with
> ENOENT but the unpatched 'rm' will go ahead and remove a/b/c (even
> though a/b/c is read-only!) whereas the patched 'rm' will diagnose the
> problem and refuse to remove a/b/c.
>
> Admittedly a contrived case, but hey! it's late.
>
> Here's a proposed patch.  Many of the changes are due to reindenting.
>
> 2007-03-07  Paul Eggert  <address@hidden>
>
>       * src/remove.c (write_protected_non_symlink): Return int, not bool,
>       so that we can indicate failure too (as a postive error number).
>       (prompt): If write_protected_non_symlink fails, report that error
>       number and fail rather than charging ahead and removing the
>       dubious entry.  Redo the logic of printing a diagnostic so that
>       we need to invoke quote (full_filename (...)) only once.
>
> diff --git a/src/remove.c b/src/remove.c
> index 97184eb..6f663ec 100644
> --- a/src/remove.c
> +++ b/src/remove.c
> @@ -704,20 +704,21 @@ is_empty_dir (int fd_cwd, char const *dir)
>    return saved_errno == 0 ? true : false;
>  }
>
> -/* Return true if FILE is determined to be an unwritable non-symlink.
> -   Otherwise, return false (including when lstat'ing it fails).
> +/* Return -1 if FILE is an unwritable non-symlink,
> +   0 if it is some other kind of file,

Thanks.
I've applied it, along with two tweaks:

Change the above comment to this:
+   0 if it is writable or some other type of file,

Move the declaration of quoted_name down to its first use.
That's ok here, since this file is already full of c99'isms.