| Message ID | 1302160948-5628-1-git-send-email-sean.finney@sonyericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sean Finney wrote:
Previously, in auth_unix_gid, group lists were stored in an array of
hard-coded length 100, and in the situation that the group lists for a
particular call were too large, the array was swapped with a dynamically
allocated/freed buffer. For environments where users are commonly in
a large number of groups, this isn't an ideal approach.
Instead, make the group list static scoped to the function, and
use malloc/realloc to grow it on an as-needed basis.
I would re-word this. The list isn't static, the list pointer is. I don't
think you need to mention that, it's just confusing. "Use malloc/realloc to
grow the list on an as-needed basis."
+ groups = malloc(sizeof(gid_t)*INITIAL_MANAGED_GROUPS);
Style nit: Put blanks around the "*".
I was going to suggest you first call getgrouplist with groups set to 0 so
you can always alloc() the correct size list, but then I found this in the
man page:
BUGS
In glibc versions before 2.3.3, the implementation of this function
contains a buffer-overrun bug: it returns the complete list of groups
for user in the array groups, even when the number of groups exceeds
*ngroups.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Apr 7, 2011, at 10:15 AM, Finney, Sean wrote: > Hi Jim, > > On Thu, 2011-04-07 at 08:22 -0400, Jim Rees wrote: > >> I was going to suggest you first call getgrouplist with groups set to 0 so >> you can always alloc() the correct size list, but then I found this in the >> man page: >> >> BUGS >> In glibc versions before 2.3.3, the implementation of this function >> contains a buffer-overrun bug: it returns the complete list of groups >> for user in the array groups, even when the number of groups exceeds >> *ngroups. > > Yuck. The function is also not in POSIX or any other standards, so > maybe it's worth discussing at a later point whether getgroups(2) would > be a better interface to use, even if it means an extra step or two. The standard RPC library function authunix_create_default() uses getgroups(2). Take a look at the glibc version of this function to see the preferred method for using getgroups(2).
Finney, Sean wrote: > I would re-word this. The list isn't static, the list pointer is. I don't > think you need to mention that, it's just confusing. "Use malloc/realloc to > grow the list on an as-needed basis." Okay. I was trying to just put a hint that there was a persistant buffer that would hang around through multiple invocations, though perhaps that's a little too much 'implementation details'. Totally fine with modifying the description either way :) If it were me, I would alloc and free the list each time rather than keeping it around. It's not like malloc is super expensive. But that's a matter of taste. > BUGS > In glibc versions before 2.3.3, the implementation of this function > contains a buffer-overrun bug: it returns the complete list of groups > for user in the array groups, even when the number of groups exceeds > *ngroups. Yuck. The function is also not in POSIX or any other standards, so maybe it's worth discussing at a later point whether getgroups(2) would be a better interface to use, even if it means an extra step or two. There's no sense in making nfs-utils portable to non-linux, so depending on glibc is probably ok. 2.3.3 was released in December 2003 so I would argue we don't care about the buffer-overrun bug. Anyway, I will re-spin this patch with the two changes that you've suggested and submit it tomorrow. Care to take a look at patch 2/2? :) It looked fine to me. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c index 9bbbfb3..7deb050 100644 --- a/utils/mountd/cache.c +++ b/utils/mountd/cache.c @@ -37,6 +37,7 @@ #include "blkid/blkid.h" #endif +#define INITIAL_MANAGED_GROUPS 100 enum nfsd_fsid { FSID_DEV = 0, @@ -127,11 +128,21 @@ void auth_unix_gid(FILE *f) */ int uid; struct passwd *pw; - gid_t glist[100], *groups = glist; - int ngroups = 100; + static gid_t *groups = NULL; + static int groups_len = 0; + gid_t *more_groups; + int ngroups = 0; int rv, i; char *cp; + if (groups_len == 0) { + groups = malloc(sizeof(gid_t)*INITIAL_MANAGED_GROUPS); + if (!groups) + return; + + groups_len = ngroups = INITIAL_MANAGED_GROUPS; + } + if (readline(fileno(f), &lbuf, &lbuflen) != 1) return; @@ -144,13 +155,16 @@ void auth_unix_gid(FILE *f) rv = -1; else { rv = getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups); - if (rv == -1 && ngroups >= 100) { - groups = malloc(sizeof(gid_t)*ngroups); - if (!groups) + if (rv == -1 && ngroups >= groups_len) { + more_groups = realloc(groups, sizeof(gid_t)*ngroups); + if (!more_groups) rv = -1; - else + else { + groups = more_groups; + groups_len = ngroups; rv = getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups); + } } } qword_printint(f, uid); @@ -162,9 +176,6 @@ void auth_unix_gid(FILE *f) } else qword_printint(f, 0); qword_eol(f); - - if (groups != glist) - free(groups); } #if USE_BLKID
Previously, in auth_unix_gid, group lists were stored in an array of hard-coded length 100, and in the situation that the group lists for a particular call were too large, the array was swapped with a dynamically allocated/freed buffer. For environments where users are commonly in a large number of groups, this isn't an ideal approach. Instead, make the group list static scoped to the function, and use malloc/realloc to grow it on an as-needed basis. Signed-off-by: Sean Finney <sean.finney@sonyericsson.com> --- utils/mountd/cache.c | 29 ++++++++++++++++++++--------- 1 files changed, 20 insertions(+), 9 deletions(-)