Re: probable big in nfs-utils

[Steve Dickson <steved@redhat.com>]

On 10/4/24 10:58 PM, Ian Kent wrote:
> Here we go again ...
> 
> On 5/10/24 10:47, Ian Kent wrote:
>> Umm, let's try that again ...
>>
>> On 5/10/24 10:41, Ian Kent wrote:
>>> Hi Steve,
>>>
>>> On 5/10/24 03:54, Steve Dickson wrote:
>>>> Hello,
>>>>
>>>> On 10/4/24 2:14 PM, Charles Hedrick wrote:
>>>>> While looking into a problem that turns out to be somewhere else, I 
>>>>> noticed that in gssd_proc.c , getpwuid is used. The context is 
>>>>> threaded, and I verified with strace that the thread is sharing 
>>>>> memory with other threads. I believe this should be changed to 
>>>>> getpwuid_r. Similarly the following call to getpwnam.
>>>>>
>>>>> Is this the right place for reports on nfs-utils?
>>>> Yes... but I'm not a fan of change code, that been around
>>>> for a while, without fixing a problem... What problem does changing
>>>> getpwuid to getpwuid_r fix?
>>>>
>>>> Patches are always welcome!
>>>>
>>>> steved.
>>>>
>>>>
>>>
>>> Yeah, getpwuid(3) and getpwnam() aren't thread safe and presumably 
>>> gssd is a service so it
>>>
>>> could have multiple concurrent callers.
>>>
>>>
> 
> [PATCH} nfs-utils: use getpwuid_r() and getpwnam_r() in gssd
> 
> 
> gssd uses getpwuid(3) and getpwnam(3) in a pthreads context but
> these functions are not thread safe.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> 
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 2ad84c59..2a376b8f 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -489,7 +489,10 @@ success:
>   static int
>   change_identity(uid_t uid)
>   {
> -       struct passwd   *pw;
> +       struct passwd  pw;
> +       struct passwd *ppw;
> +       char *pw_tmp;
> +       long tmplen;
>          int res;
> 
>          /* drop list of supplimentary groups first */
> @@ -502,15 +505,25 @@ change_identity(uid_t uid)
>                  return errno;
>          }
> 
> +       tmplen = sysconf(_SC_GETPW_R_SIZE_MAX);
> +       if (tmplen < 0)
> +               bufsize = 16384;
> +
> +       pw_tmp = malloc(tmplen);
> +       if (!pw_tmp) {
> +               printerr(0, "WARNING: unable to allocate passwd buffer\n");
> +               return errno ? errno : ENOMEM;
> +       }
> +
>          /* try to get pwent for user */
> -       pw = getpwuid(uid);
> -       if (!pw) {
> +       res = getpwuid_r(uid, &pw, pw_tmp, tmplen, &ppw);
> +       if (!ppw) {
>                  /* if that doesn't work, try to get one for "nobody" */
> -               errno = 0;
> -               pw = getpwnam("nobody");
> -               if (!pw) {
> +               res = getpwnam_r("nobody", &pw, pw_tmp, tmplen, &ppw);
> +               if (!ppw) {
>                          printerr(0, "WARNING: unable to determine gid 
> for uid %u\n", uid);
> -                       return errno ? errno : ENOENT;
> +                       free(pw_tmp);
> +                       return res ? res : ENOENT;
>                  }
>          }
> 
> @@ -521,12 +534,13 @@ change_identity(uid_t uid)
>           * other threads. To bypass this, we have to call syscall() 
> directly.
>           */
>   #ifdef __NR_setresgid32
> -       res = syscall(SYS_setresgid32, pw->pw_gid, pw->pw_gid, pw->pw_gid);
> +       res = syscall(SYS_setresgid32, pw.pw_gid, pw.pw_gid, pw.pw_gid);
>   #else
> -       res = syscall(SYS_setresgid, pw->pw_gid, pw->pw_gid, pw->pw_gid);
> +       res = syscall(SYS_setresgid, pw.pw_gid, pw.pw_gid, pw.pw_gid);
>   #endif
> +       free(pw_tmp);
>          if (res != 0) {
> -               printerr(0, "WARNING: failed to set gid to %u!\n", pw- 
>  >pw_gid);
> +               printerr(0, "WARNING: failed to set gid to %u!\n", 
> pw.pw_gid);
>                  return errno;
>          }
> 
> 
checking file utils/gssd/gssd_proc.c
Hunk #1 FAILED at 489.
Hunk #2 FAILED at 502.
Hunk #3 FAILED at 521.
3 out of 3 hunks FAILED

What branch are you applying this patch to?
Maybe it is me copying the patch over...

Try git format-patch that seems to work.

steved.