Re: probable big in nfs-utils

[Steve Dickson <steved@redhat.com>]

On 10/20/24 8:46 PM, Ian Kent wrote:
> 
> On 19/10/24 20:40, Steve Dickson wrote:
>>
>>
>> On 10/18/24 7:19 PM, Ian Kent wrote:
>>> On 18/10/24 21:10, Steve Dickson wrote:
>>>> 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".
>>>
>>> Sure I can do that.
>>>
>>> I'll do it over the weekend.
>>>
>>>
>>>>
>>>> I am assuming the patch is tested ;-)
>>>
>>> It was just an example so it hasn't been tested.
>> I'm testing it now... it seems stable.
> 
> Right, I say I didn't test it but that means I didn't test the code in
> 
> place. I cut and pasted the bits of it into a simple program and checked
> 
> it did what it was meant to do.
> 
> 
> I didn't feel I could claim I tested it but it should (and sounds like
> 
> it did) work ok because I could have made mistakes with this.
> 
> 
> Ian
> 
>>
>> thanks!
>>
>> steved.
>>>
>>>
>>> It was taken from code that I have in autofs that has been in use for
>>>
>>> many years and I'm pretty sure I saw similar code already in nfs-utils
>>>
>>> so the code pattern is well established. Some care should be sufficient
>>>
>>> to screw it up.
>>>
>>>
>>> Anyway I'll post it so it can be reviewed by others that are familiar
>>>
>>> with the code pattern.
No worries... The new release, which includes the patch is
making it through Bake-a-ton testing with flying colors!

steved.
>>>
>>>
>>> Ian
>>>
>>>> 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
>>>>>
>>>>
>>>
>>
>