Re: [PATCH] fix NFSv4 acl detection on F39

[Paul Eggert]

What Bruno said, plus:

On 2023-05-01 12:43, Ondrej Valousek wrote:

> +/* Return 1 if ATTR is found in the xattr list given by BUF */
> +int have_xattr (char const *attr, char *buf, ssize_t buflen)

This returns 1 or 0 so let's have it return bool instead. Also, it 
doesn't modify the buffer (or at least, it won't once you get rid of the 
'free'), so change it to 'char const *buf'.

> +    int keylen;

This should be size_t, not int. Also, declare it when initialized, not here.

> +    if (strcmp (key, attr) == 0)
> +      {
> +        /* we don't expect this function will be called again
> +           so we free the buffer so caller does not have to */
> +        free (buf);
> +        return 1;
> +      }
> +    keylen = strlen (key) + 1;

I found have_xattr a bit confusing. How about something like this instead?

  static bool
  have_xattr (char const *attr, char const *buf, ssize_t buflen)
  {
    char const *key = buf;
    char const *buflim = buf + buflen;
    for (char const *key = buf; key < buflim; key += strlen (key) + 1)
      if (strcmp (key, attr) == 0)
        return true;
    return false;
  }



> +      attrlist = malloc (ret);
> +      if (attrlist == NULL)
> +        return -1;
> +      buflen = listxattr (name, attrlist, ret);

Shouldn't this be llistxattr, not listxattr?

Let's declare attrlist and buflen when initialized, e.g., "char 
*attrlist = malloc (ret);".

> +      if (buflen == -1)

This should be "buflen < 0".

More important, there's a race condition here, as someone could add 
attributes to the file between the two listxattr calls. So I suggest 
using this algorithm instead:

* Do not use llistxattr (name, NULL, 0). Instead, invoke llistxattr on a 
small (say, 3 KiB) buffer on the stack. Use malloc only if llistxattr 
returns ERANGE, and keep expanding this buffer (via free-then-malloc, 
not realloc, since you don't need to save the old storage) while 
llistxattr returns ERANGE. Check for integer overflow when multiplying 
the buffer size by 1.5, by using ckd_add. Use 'free' at the end only if 
we used 'malloc'.

> +      if(have_xattr (XATTR_NAME_POSIX_ACL_ACCESS, attrlist, buflen))
>          return 1;
> +      else
> +        ret = 0;

Omit "else".


> +          if (have_xattr (XATTR_NAME_POSIX_ACL_DEFAULT, attrlist, buflen))
>               return 1;
> +          else
> +            ret = 0;

RET is already zero here, so omit "else ret = 0;".


> -      if (ret < 0)
> +      if (have_xattr (XATTR_NAME_NFSV4_ACL, attrlist, buflen))
>           {
>             /* Check for NFSv4 ACLs.  The max length of a trivial
>                ACL is 6 words for owner, 6 for group, 7 for everyone,

The "if" part here goes on to call getxattr, but we don't want that as 
it's both slower and a race condition: we want the attribute we already 
got via llistxattr.