bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux


From: Bruno Haible
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux
Date: Fri, 02 Dec 2022 01:58:30 +0100

Hi Ondrej,

Still some formatting issues. But now, with formatting nearly right, I'm able
to spot other things too.

> +# define ACE4_WHO_OWNER    "OWNER@"
> +# define ACE4_WHO_GROUP    "GROUP@"
> +# define ACE4_WHO_EVERYONE "EVERYONE@"
> +
> +# define ACE4_ACCESS_ALLOWED_ACE_TYPE   0
> +# define ACE4_ACCESS_DENIED_ACE_TYPE    1
> +
> +/* ACE flag values */
> +# define ACE4_IDENTIFIER_GROUP          0x00000040
> +# define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))

These are all inside a '# if' / '# endif' pair, so the preprocessor statement
indentation level is 2. Please add one more space between '#' and 'define' here.

> +int
> +acl_nfs4_nontrivial (char *xattr, int len)
> +{
> +  int ret = 0,
> +      wholen,
> +      bufs = len;
> +  uint32_t flag,
> +           type,
> +           num_aces = ntohl (*((uint32_t*)(xattr))); /* Grab the number of 
> aces in the acl */
> +  char *bufp = xattr;
> +  int num_a_aces = 0,
> +      num_d_aces = 0;
> +
> +  ret = 0;

'ret' is already initialized to 0 above. One of the two initializations is
redundant.

Hmm... is the variable 'ret' used at all? You should routinely compile
with "gcc -Wall"; this catches unused variables, usually.

> +  bufp += 4;  /* sizeof(uint32_t); */
> +  bufs -= 4;
> +
> +  for (uint32_t ace_n = 0; num_aces > ace_n ; ace_n++)
> +    {
> +      int d_ptr;

There are more variables which can be moved into the 'for' loop. Generally,
moving variables into the smallest scope that they need provides for a better
readability of the code. If I'm not mistaken, here, it applies to the variables
wholen, flag, type.

> +
> +      /* Get the acl type */
> +      if (bufs <= 0)
> +        return -1;
> +
> +      type = ntohl (*((uint32_t*)bufp));
> +
> +      bufp += 4;
> +      bufs -= 4;
> +      if (bufs <= 0)
> +        return -1;
> +
> +      flag = ntohl (*((uint32_t*)bufp));
> +      /* As per RFC 7530, the flag should be 0, but we are just generous to 
> Netapp
> +       * and also accept the Group flag
> +       */
> +      if (flag & ~ACE4_IDENTIFIER_GROUP)
> +        return 1;
> +
> +      /* we skip mask -
> +       * it's too risky to test it and it does not seem to be actually 
> needed */
> +      bufp += 2*4;
> +      bufs -= 2*4;
> +
> +      if (bufs <= 0)
> +        return -1;
> +
> +      wholen = ntohl (*((uint32_t*)bufp));
> +
> +      bufp += 4;
> +      bufs -= 4;
> +
> +      /* Get the who string */
> +      if (bufs <= 0)
> +        return -1;
> +
> +      /* for trivial ACL, we expect max 5 (typically 3) ACES, 3 Allow, 2 
> deny */
> +      if (((strncmp (bufp,ACE4_WHO_OWNER,wholen) == 0) ||
> +          (strncmp (bufp,ACE4_WHO_GROUP,wholen) == 0)) &&
> +           wholen == 6)

Please move the || and && operators to the next line:

         if (((strncmp (bufp,ACE4_WHO_OWNER,wholen) == 0)
              || (strncmp (bufp,ACE4_WHO_GROUP,wholen) == 0))
             && wholen == 6)

Add add a space after each comma in an argument list.

> +        {
> +          if (type == ACE4_ACCESS_ALLOWED_ACE_TYPE)
> +            num_a_aces++;
> +          if (type == ACE4_ACCESS_DENIED_ACE_TYPE)
> +            num_d_aces++;
> +        } else

Please put the closing brace and the 'else' on separate lines.

> +          if ((strncmp (bufp,ACE4_WHO_EVERYONE,wholen) == 0)
> +              && (type == ACE4_ACCESS_ALLOWED_ACE_TYPE)
> +              && (wholen == 9))
> +            num_a_aces++;
> +          else
> +            return 1;
> +
> +      d_ptr = ROUNDUP (wholen,4);
> +      bufp += d_ptr;
> +      bufs -= d_ptr;
> +
> +      /* Make sure we aren't outside our domain */
> +      if (bufs < 0)
> +        return -1;
> +
> +    }
> +  return (num_a_aces <= 3) && (num_d_aces <= 2)
> +         && (num_a_aces + num_d_aces == num_aces) ? 0 : 1;

You can simplify this: Knowing that boolean expressions have the value 1
for true and 0 for false in C, you can write it as

     return !((num_a_aces <= 3) && (num_d_aces <= 2)
              && (num_a_aces + num_d_aces == num_aces));

which is smaller and simpler.

> diff --git a/lib/acl-internal.h b/lib/acl-internal.h
> index 94553fab2..b674c7702 100644
> --- a/lib/acl-internal.h
> +++ b/lib/acl-internal.h
> @@ -146,6 +146,9 @@ rpl_acl_set_fd (int fd, acl_t acl)
>  #   define acl_entries rpl_acl_entries
>  extern int acl_entries (acl_t);
>  #  endif
> +/* Return 1 if given ACL in XDR format is non-trivial
> + * Return 0 if it is trivial */
> +extern int acl_nfs4_nontrivial (char *,int);

Please add a space after the comma.

> diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
> index e02f0626a..82830ba4a 100644
> --- a/lib/file-has-acl.c
> +++ b/lib/file-has-acl.c
> @@ -32,6 +32,11 @@
>  #if GETXATTR_WITH_POSIX_ACLS
>  # include <sys/xattr.h>
>  # include <linux/xattr.h>
> +# include <arpa/inet.h>
> +#ifndef XATTR_NAME_NFSV4_ACL
> +# define XATTR_NAME_NFSV4_ACL "system.nfs4_acl"
> +#endif

Please indent the last three of these lines by one more space, after the '#'.

Bruno






reply via email to

[Prev in Thread] Current Thread [Next in Thread]