Re: [PATCH v2] rmdir: diagnose non following of symlinks with trailing slash

[Bernhard Voelker]

On 2/17/21 5:32 PM, Pádraig Brady wrote:
> Subject: [PATCH] rmdir: diagnose non following of symlinks with trailing slash
>
> Linux is unusual here in that rmdir("symlink/") returns ENOTDIR,
__^^^^^
Shouldn't we better use GNU/Linux in all places?

> whereas Solaris and FreeBSD at least, will follow the symlink
> and remove the target directory.  We don't make the behviour

s/behvio/behavio/

> on Linux consistent, but at least clarify the confusing error message.

> diff --git a/src/rmdir.c b/src/rmdir.c
> index dffe24bc7..3a9911ff0 100644
> --- a/src/rmdir.c
> +++ b/src/rmdir.c
> @@ -238,9 +248,42 @@ main (int argc, char **argv)
>            if (ignorable_failure (rmdir_errno, dir))
>              continue;
>
> -          /* Here, the diagnostic is less precise, since we have no idea
> -             whether DIR is a directory.  */
> -          error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
> +          /* Distinguish the case for a symlink with trailing slash.
> +             On Linux, rmdir(2) confusingly does not follow the symlink,
> +             thus giving the errno ENOTDIR, while on other systems the 
> symlink
> +             is followed.  We don't provide consistent behavior here,
> +             but at least we provide a more accurate error message.  */
> +          bool custom_error = false;
> +          if (rmdir_errno == ENOTDIR)
> +            {
> +              char const *last_unix_slash = strrchr (dir, '/');
> +              if (last_unix_slash && (*(last_unix_slash + 1) == '\0'))
> +                {
> +                  struct stat st;
> +                  int ret = stat (dir, &st);
> +                  /* Some other issue following, or is actually a directory. 
> */
> +                  if ((ret != 0 && errno != ENOTDIR) || S_ISDIR (st.st_mode))
> +                    {
> +                      /* Ensure the last component was a symlink.  */
> +                      char* dir_arg = xstrdup (dir);
> +                      strip_trailing_slashes (dir);
> +                      ret = lstat (dir, &st);
> +                      if (ret == 0 && S_ISLNK (st.st_mode))
> +                        {
> +                          error (0, 0,
> +                                 _("failed to remove %s:"
> +                                   " Symbolic link not followed"),
> +                                 quoteaf (dir_arg));
> +                          custom_error = true;
> +                        }
> +                      free (dir_arg);
> +                    }
> +                }
> +            }
> +
> +          if (! custom_error)
> +            error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
> +
>            ok = false;
>          }
>        else if (remove_empty_parents)
>

Hmm, that's quite some code and 1x stat + 1x lstat - just to massage parts of 
the
error diagnostic.  And it opens a new tiny race window between rmdir and 
stat+lstat.

What if we remove any trailing slashes instead on systems where rmdir(2) 
wouldn't follow
a symlink?

Have a nice day,
Berny