probable big in nfs-utils

probable big in nfs-utils

[Charles Hedrick]

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?

Re: probable big in nfs-utils

[Steve Dickson]

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.

Re: probable big in nfs-utils

[Ian Kent]

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.


You could use domething like this ...


nfs-utils: use getpwuid_r() and getpwnam_r() in gssd From: Ian Kent 
<raven@themaw.net> 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> --- utils/gssd/gssd_proc.c | 28 
+++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 
deletions(-) diff --git a/utils/gssd/gssd_proc.c 
b/utils/gssd/gssd_proc.c index 2ad84c59..c718be6f 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; } } @@ -525,6 +538,7 @@ 
change_identity(uid_t uid) #else 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); 
return errno;

Re: probable big in nfs-utils

[Ian Kent]

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.
>
>
> You could use domething like this ...
>
>
> nfs-utils: use getpwuid_r() and getpwnam_r() in gssd From: Ian Kent 
> <raven@themaw.net> 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> --- utils/gssd/gssd_proc.c | 28 
> +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 
> deletions(-) diff --git a/utils/gssd/gssd_proc.c 
> b/utils/gssd/gssd_proc.c index 2ad84c59..c718be6f 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; } } @@ -525,6 +538,7 @@ change_identity(uid_t uid) #else 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); return errno;
>
>

You could use something like this ...


nfs-utils: use getpwuid_r() and getpwnam_r() in gssd

From: Ian Kent <raven@themaw.net>

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>
---
  utils/gssd/gssd_proc.c |   28 +++++++++++++++++++++-------
  1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 2ad84c59..c718be6f 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;
                 }
         }

@@ -525,6 +538,7 @@ change_identity(uid_t uid)
  #else
         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);
                 return errno;

Re: probable big in nfs-utils

[Ian Kent]

On 5/10/24 10:47, Ian Kent wrote:
> Umm, let's try that again ...

Actually that's not quite right.


>
> 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.
>>
>>
>> You could use domething like this ...
>>
>>
>> nfs-utils: use getpwuid_r() and getpwnam_r() in gssd From: Ian Kent 
>> <raven@themaw.net> 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> --- utils/gssd/gssd_proc.c 
>> | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 
>> deletions(-) diff --git a/utils/gssd/gssd_proc.c 
>> b/utils/gssd/gssd_proc.c index 2ad84c59..c718be6f 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; } } @@ -525,6 +538,7 @@ change_identity(uid_t uid) #else 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); return errno;
>>
>>
>
> You could use something like this ...
>
>
> nfs-utils: use getpwuid_r() and getpwnam_r() in gssd
>
> From: Ian Kent <raven@themaw.net>
>
> 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>
> ---
>  utils/gssd/gssd_proc.c |   28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 2ad84c59..c718be6f 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;
>                 }
>         }
>
> @@ -525,6 +538,7 @@ change_identity(uid_t uid)
>  #else
>         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);
>                 return errno;
>
>

Re: probable big in nfs-utils

[Ian Kent]

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;
         }

Re: probable big in nfs-utils

[Steve Dickson]

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.

Re: probable big in nfs-utils

[Ian Kent]

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

Re: probable big in nfs-utils

[Steve Dickson]

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
> 

Re: probable big in nfs-utils

[Ian Kent]

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.


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

Re: probable big in nfs-utils

[Steve Dickson]

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

Re: probable big in nfs-utils

[Ian Kent]

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

Re: probable big in nfs-utils

[Steve Dickson]

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