Re: [PATCH] libacl: use getpwnam_r and getgrnam_r in acl_from_text.c

[Andreas Grünbacher]

Hi Lukáš,

Am Mi., 20. Juli 2022 um 16:35 Uhr schrieb <lzaoral@redhat.com>:
> From: Lukáš Zaoral <lzaoral@redhat.com>
>
> Plain getpwnam and getgrnam are not thread safe.

looking good in principle. A few comments below ...

> Fixes: #62371
> ---
>  libacl/acl_from_text.c | 50 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/libacl/acl_from_text.c b/libacl/acl_from_text.c
> index 2617cbb..b46e694 100644
> --- a/libacl/acl_from_text.c
> +++ b/libacl/acl_from_text.c
> @@ -23,6 +23,7 @@
>  #include <string.h>
>  #include <pwd.h>
>  #include <grp.h>
> +#include <unistd.h>
>  #include "libacl.h"
>  #include "misc.h"
>
> @@ -146,21 +147,45 @@ get_id(const char *token, id_t *id_p)
>  }
>
>
> +static int
> +allocate_buffer(char **buffer, long *bufsize, int type)

The buffer size should be a size_t.

> +{
> +       *bufsize = sysconf(type);
> +       if (*bufsize == -1)
> +               *bufsize = 16384;
> +
> +       *buffer = malloc(*bufsize);
> +       if (*buffer == NULL)
> +               return -1;
> +
> +       return 0;
> +}

Can we use alloca() in the caller instead of malloc() here? Can the
the default buffer size be passed in?

libacl is using getpwuid() and getgrgid() as well, which need the same
treatment.

tools/parse.c (setfacl) is using getpwnam(), getgrnam, getpwuid(), and
getgrgid() as well. I know that setfacl is single-threaded, but
wouldn't it make sense to switch to the _r functions there as well
while we're at it?

For that to work, it may make sense to add some helper code to libmisc.

> +
> +
>  static int
>  get_uid(const char *token, uid_t *uid_p)
>  {
> -       struct passwd *passwd;
> +       struct passwd passwd;
> +       struct passwd *result;
> +       char * buffer;
> +       long bufsize;
>
>         if (get_id(token, uid_p) == 0)
>                 return 0;
> +
> +       if (allocate_buffer(&buffer, &bufsize, _SC_GETPW_R_SIZE_MAX) != 0)
> +               return -1;
> +
>         errno = 0;
> -       passwd = getpwnam(token);
> -       if (passwd) {
> -               *uid_p = passwd->pw_uid;
> +       getpwnam_r(token, &passwd, buffer, bufsize, &result);
> +       if (result) {
> +               *uid_p = passwd.pw_uid;
> +               free(buffer);
>                 return 0;
>         }
>         if (errno == 0)
>                 errno = EINVAL;
> +       free(buffer);
>         return -1;
>  }
>
> @@ -168,18 +193,27 @@ get_uid(const char *token, uid_t *uid_p)
>  static int
>  get_gid(const char *token, gid_t *gid_p)
>  {
> -       struct group *group;
> +       struct group group;
> +       struct group *result;
> +       char * buffer;
> +       long bufsize;
>
>         if (get_id(token, (uid_t *)gid_p) == 0)
>                 return 0;
> +
> +       if (allocate_buffer(&buffer, &bufsize, _SC_GETGR_R_SIZE_MAX) != 0)
> +               return -1;
> +
>         errno = 0;
> -       group = getgrnam(token);
> -       if (group) {
> -               *gid_p = group->gr_gid;
> +       getgrnam_r(token, &group, buffer, bufsize, &result);
> +       if (result) {
> +               *gid_p = group.gr_gid;
> +               free(buffer);
>                 return 0;
>         }
>         if (errno == 0)
>                 errno = EINVAL;
> +       free(buffer);
>         return -1;
>  }
>
> --
> 2.36.1
>
>

Thanks,
Andreas