Re: probable big in nfs-utils

[Steve Dickson <steved@redhat.com>]

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.

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