Re: probable big in nfs-utils

[Steve Dickson <steved@redhat.com>]

Hey Ian,

I would like to get this into the up
coming nfs-utils release for the Bakeathon
next week. Would mind reformatting the
patch (dos2unix didn't work) and
resubmit using "git format-patch"
and "git send-email".

I am assuming the patch is tested ;-)

tia,

steved.

On 10/6/24 7:53 PM, Ian Kent wrote:
> On 6/10/24 20:43, Steve Dickson wrote:
>>
>>
>> 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.
> 
> Opps, sorry!
> 
> I thought it was the nfs-utils repo. main branch ...
> 
> 
> Maybe the patch has DOS carriage controls, I see that a lot myself.
> 
> If a simple dos2unix doesn't fix it I'll start checking my end and use the
> 
> "git format-patch", "git send-email" pair.
> 
> 
> Ian
>