* probable big in nfs-utils @ 2024-10-04 18:14 Charles Hedrick 2024-10-04 19:54 ` Steve Dickson 0 siblings, 1 reply; 13+ messages in thread From: Charles Hedrick @ 2024-10-04 18:14 UTC (permalink / raw) To: linux-nfs@vger.kernel.org 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-04 18:14 probable big in nfs-utils Charles Hedrick @ 2024-10-04 19:54 ` Steve Dickson 2024-10-05 2:41 ` Ian Kent 0 siblings, 1 reply; 13+ messages in thread From: Steve Dickson @ 2024-10-04 19:54 UTC (permalink / raw) To: Charles Hedrick, linux-nfs@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-04 19:54 ` Steve Dickson @ 2024-10-05 2:41 ` Ian Kent 2024-10-05 2:47 ` Ian Kent 0 siblings, 1 reply; 13+ messages in thread From: Ian Kent @ 2024-10-05 2:41 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-05 2:41 ` Ian Kent @ 2024-10-05 2:47 ` Ian Kent 2024-10-05 2:52 ` Ian Kent 2024-10-05 2:58 ` Ian Kent 0 siblings, 2 replies; 13+ messages in thread From: Ian Kent @ 2024-10-05 2:47 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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; ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-05 2:47 ` Ian Kent @ 2024-10-05 2:52 ` Ian Kent 2024-10-05 2:58 ` Ian Kent 1 sibling, 0 replies; 13+ messages in thread From: Ian Kent @ 2024-10-05 2:52 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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; > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-05 2:47 ` Ian Kent 2024-10-05 2:52 ` Ian Kent @ 2024-10-05 2:58 ` Ian Kent 2024-10-06 12:43 ` Steve Dickson 1 sibling, 1 reply; 13+ messages in thread From: Ian Kent @ 2024-10-05 2:58 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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; } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-05 2:58 ` Ian Kent @ 2024-10-06 12:43 ` Steve Dickson 2024-10-06 23:53 ` Ian Kent 0 siblings, 1 reply; 13+ messages in thread From: Steve Dickson @ 2024-10-06 12:43 UTC (permalink / raw) To: Ian Kent, Charles Hedrick, linux-nfs@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-06 12:43 ` Steve Dickson @ 2024-10-06 23:53 ` Ian Kent 2024-10-18 13:10 ` Steve Dickson 0 siblings, 1 reply; 13+ messages in thread From: Ian Kent @ 2024-10-06 23:53 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-06 23:53 ` Ian Kent @ 2024-10-18 13:10 ` Steve Dickson 2024-10-18 23:19 ` Ian Kent 0 siblings, 1 reply; 13+ messages in thread From: Steve Dickson @ 2024-10-18 13:10 UTC (permalink / raw) To: Ian Kent, Charles Hedrick, linux-nfs@vger.kernel.org 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-18 13:10 ` Steve Dickson @ 2024-10-18 23:19 ` Ian Kent 2024-10-19 12:40 ` Steve Dickson 0 siblings, 1 reply; 13+ messages in thread From: Ian Kent @ 2024-10-18 23:19 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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 >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-18 23:19 ` Ian Kent @ 2024-10-19 12:40 ` Steve Dickson 2024-10-21 0:46 ` Ian Kent 0 siblings, 1 reply; 13+ messages in thread From: Steve Dickson @ 2024-10-19 12:40 UTC (permalink / raw) To: Ian Kent, Charles Hedrick, linux-nfs@vger.kernel.org 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 >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-19 12:40 ` Steve Dickson @ 2024-10-21 0:46 ` Ian Kent 2024-10-23 0:51 ` Steve Dickson 0 siblings, 1 reply; 13+ messages in thread From: Ian Kent @ 2024-10-21 0:46 UTC (permalink / raw) To: Steve Dickson, Charles Hedrick, linux-nfs@vger.kernel.org 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 >>>> >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: probable big in nfs-utils 2024-10-21 0:46 ` Ian Kent @ 2024-10-23 0:51 ` Steve Dickson 0 siblings, 0 replies; 13+ messages in thread From: Steve Dickson @ 2024-10-23 0:51 UTC (permalink / raw) To: Ian Kent, Charles Hedrick, linux-nfs@vger.kernel.org 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 >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-10-23 0:51 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-04 18:14 probable big in nfs-utils Charles Hedrick 2024-10-04 19:54 ` Steve Dickson 2024-10-05 2:41 ` Ian Kent 2024-10-05 2:47 ` Ian Kent 2024-10-05 2:52 ` Ian Kent 2024-10-05 2:58 ` Ian Kent 2024-10-06 12:43 ` Steve Dickson 2024-10-06 23:53 ` Ian Kent 2024-10-18 13:10 ` Steve Dickson 2024-10-18 23:19 ` Ian Kent 2024-10-19 12:40 ` Steve Dickson 2024-10-21 0:46 ` Ian Kent 2024-10-23 0:51 ` Steve Dickson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox