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

[Pádraig Brady]

On 18/02/2021 23:09, Bernhard Voelker wrote:
> 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?

I was focusing on the kernel here.
I will be more explicit.

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

ack


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

Sorry, I don't understand your suggestion.

The second lstat isn't strictly necessary,
but more defensive programming.
The extra stats are only in the error path and so not a perf issue I think.
Any races would be very edge case, and only impact the error, in which case
the "Symbolic link not followed" would be correct for a subsequent run anyway.

thanks for the review,
Pádraig