Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L

[Bruno Haible]

Kamil Dudka wrote:
> >   2) see a test case added to gnulib or coreutils.
> 
> A while ago, Jim wrote a test-case for coreutils that catches exactly the bug 
> that the original patch introduced.

I'm asking for a test case also for the bug that the original patch fixed.

> > If you cannot come up with such a test case, I suggest to revert the patch
> > of 2011-07-22.
> 
> Why?  AFAIK, there has never been a test-case for the ACL detection in ls.

The complexity of this file_has_acl business starts to become complicated.
We are discussing regular files, symlinks, symlinks to mounpoints, etc.

> Note the bug above has a Fedora sibling with more background included:
> 
> https://bugzilla.redhat.com/692823
> ...
> > are you saying in <https://bugzilla.redhat.com/720325> that
> > calling acl_extended_file on a non-symlink will trigger autofs mounts??
> 
> Yes, the getxattr syscall triggers it.  lgetxattr does not.

Thanks for that background. So we're looking at "ls -l /media" or
"ls -ld mountpoint". Since you've mentioned symlinks, we also have to
consider symlinks to mountpoints.

So in fact we have 9 cases:
  a) A non-symlink, non-directory. Here acl_extended_file_nofollow and
     acl_extended_file are equivalent.
  b) A directory that is not a mount point.
  c) A mount point.
  d) A symlink to a file, with dereferencing (stat()).
  e) A symlink to a directory that is not a mount point, with dereferencing 
(stat()).
  f) A symlink to a mount point, with dereferencing (stat()).
  g) A symlink to a file, without dereferencing (lstat()).
  h) A symlink to a directory that is not a mount point, without dereferencing 
(lstat()).
  i) A symlink to a mount point, without dereferencing (lstat()).

Remember the option '-L' of 'ls' is meant to mean dereference a symbolic link,
but does not mean a different treatment of mount points. But in the kernel,
the difference between getxattr and lgetxattr is a flags word of LOOKUP_FOLLOW
vs. 0 and - apparently, like you say - makes a difference for mount points.

So how do these 9 cases need to be handled:
  a) Either acl_extended_file_nofollow or acl_extended_file will do.
  b) Either acl_extended_file_nofollow or acl_extended_file will do.
  c) Must call acl_extended_file_nofollow.
  d) Either use acl_extended_file, or find the target filename through
     canonicalize_filename_mode(CAN_EXISTING), then acl_extended_file_nofollow.
  e) Either detect the non-mount-point through statvfs() or similar then call
     acl_extended_file, or must find the target filename through
     canonicalize_filename_mode(CAN_EXISTING) then call 
acl_extended_file_nofollow.
  f) Must find the target filename through
     canonicalize_filename_mode(CAN_EXISTING), then acl_extended_file_nofollow.
  g) Must return 0.
  h) Must return 0.
  i) Must return 0.

The original code mishandled the cases c), e), f).
The patch from 2011-07-22 corrected case c) and mishandled the cases d), e), f).
Your current patch corrects the cases d), e), but still mishandles case f).
And it adds an extra, unnecessary system call in case a).

> > , when it is not necessary.
> 
> I disagree.  ls -L does not do lstat().

I'm trying to avoid useless system calls. If
   ! S_ISLNK (sb->st_mode) && ! S_ISDIR (sb->st_mode)
then we are in case a) or d), and then no additional lstat() call is needed.
Since case a) is the most frequent one, I would find it worth to do this
check for S_ISDIR (sb->st_mode) since it can save an lstat() call.

So I'm asking to change

    ret = acl_extended_file_wrap (name);

to

    if (S_ISDIR (sb->st_mode))
      ret = acl_extended_file_wrap (name);
    else
      ret = acl_extended_file (name);

But still still does not fix the problem of case f). Here, with your current
patch, "ls -lLd symlink-to-mountpoint" will call acl_extended_file(), which
will call getxattr(), which will activate an autofs mount.

IMO, there are two solutions to this:
  - Either convince the kernel people to introduce a different flag, keeping
    LOOKUP_FOLLOW for symlinks only,
  - Or use code equivalent to canonicalize_filename_mode(CAN_EXISTING).

Bruno