From e14ea3f5c37475e12a3b7fb7bd3165b0a4e77c51 Mon Sep 17 00:00:00 2001 From: Csaba Henk Date: Wed, 5 Jul 2017 17:48:37 +0200 Subject: groups: don't allocate auxiliary gid list on stack When glusterfs wants to retrieve the list of auxiliary gids of a user, it typically allocates a sufficiently big gid_t array on stack and calls getgrouplist(3) with it. However, "sufficiently big" means to be of maximum supported gid list size, which in GlusterFS is GF_MAX_AUX_GROUPS = 64k. That means a 64k * sizeof(gid_t) = 256k allocation, which is big enough to overflow the stack in certain cases. A further observation is that stack allocation of the gid list brings no gain, as in all cases the content of the gid list eventually gets copied over to a heap allocated buffer. So we add a convenience wrapper of getgrouplist to libglusterfs called gf_getgrouplist which calls getgrouplist with a sufficiently big heap allocated buffer (it takes care of the allocation too). We are porting all the getgrouplist invocations to gf_getgrouplist and thus eliminate the huge stack allocation. BUG: 1464327 Change-Id: Icea76d0d74dcf2f87d26cb299acc771ca3b32d2b Signed-off-by: Csaba Henk Reviewed-on: https://review.gluster.org/17706 Smoke: Gluster Build System Reviewed-by: Niels de Vos Reviewed-by: Amar Tumballi CentOS-regression: Gluster Build System --- libglusterfs/src/common-utils.c | 73 +++++++++++++++++++++++++++++++++++++++++ libglusterfs/src/common-utils.h | 3 ++ libglusterfs/src/glusterfs.h | 3 -- libglusterfs/src/stack.h | 15 ++++++--- 4 files changed, 86 insertions(+), 8 deletions(-) (limited to 'libglusterfs') diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c index 5015d9666b6..e5e08909ac7 100644 --- a/libglusterfs/src/common-utils.c +++ b/libglusterfs/src/common-utils.c @@ -30,6 +30,7 @@ #include #include #include /* for dirname() */ +#include #if defined(GF_BSD_HOST_OS) || defined(GF_DARWIN_HOST_OS) #include @@ -4877,3 +4878,75 @@ close_fds_except (int *fdv, size_t count) #endif /* !GF_LINUX_HOST_OS */ return 0; } + +/** + * gf_getgrouplist - get list of groups to which a user belongs + * + * A convenience wrapper for getgrouplist(3). + * + * @param user - same as in getgrouplist(3) + * @param group - same as in getgrouplist(3) + * @param groups - pointer to a gid_t pointer + * + * gf_getgrouplist allocates a gid_t buffer which is big enough to + * hold the list of auxiliary group ids for user, up to the GF_MAX_AUX_GROUPS + * threshold. Upon succesfull invocation groups will be pointed to that buffer. + * + * @return success: the number of auxiliary group ids retrieved + * failure: -1 + */ +int +gf_getgrouplist (const char *user, gid_t group, gid_t **groups) +{ + int ret = -1; + int ngroups = SMALL_GROUP_COUNT; + + *groups = GF_CALLOC (sizeof (gid_t), ngroups, gf_common_mt_groups_t); + if (!*groups) + return -1; + + /* + * We are running getgrouplist() in a loop until we succeed (or hit + * certain exit conditions, see the comments below). This is because + * the indicated number of auxiliary groups that we obtain in case of + * the failure of the first invocation is not guaranteed to keep its + * validity upon the next invocation with a gid buffer of that size. + */ + for (;;) { + int ngroups_old = ngroups; + ret = getgrouplist (user, group, *groups, &ngroups); + if (ret != -1) + break; + + if (ngroups >= GF_MAX_AUX_GROUPS) { + /* + * This should not happen as GF_MAX_AUX_GROUPS is set + * to the max value of number of supported auxiliary + * groups across all platforms supported by GlusterFS. + * However, if it still happened some way, we wouldn't + * care about the incompleteness of the result, we'd + * just go on with what we got. + */ + return GF_MAX_AUX_GROUPS; + } else if (ngroups <= ngroups_old) { + /* + * There is an edge case that getgrouplist() fails but + * ngroups remains the same. This is actually not + * specified in getgrouplist(3), but implementations + * can do this upon internal failure[1]. To avoid + * falling into an infinite loop when this happens, we + * break the loop if the getgrouplist call failed + * without an increase in the indicated group number. + * + * [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=grp/initgroups.c;hb=refs/heads/release/2.25/master#l168 + */ + GF_FREE (*groups); + return -1; + } + + *groups = GF_REALLOC (*groups, ngroups * sizeof (gid_t)); + if (!*groups) + return -1; + } + return ret; +} diff --git a/libglusterfs/src/common-utils.h b/libglusterfs/src/common-utils.h index c3c5ec77350..6a260c090c8 100644 --- a/libglusterfs/src/common-utils.h +++ b/libglusterfs/src/common-utils.h @@ -918,4 +918,7 @@ get_ip_from_addrinfo (struct addrinfo *addr, char **ip); int close_fds_except (int *fdv, size_t count); + +int +gf_getgrouplist (const char *user, gid_t group, gid_t **groups); #endif /* _COMMON_UTILS_H */ diff --git a/libglusterfs/src/glusterfs.h b/libglusterfs/src/glusterfs.h index ee5c21d43d5..2cd1f363b3b 100644 --- a/libglusterfs/src/glusterfs.h +++ b/libglusterfs/src/glusterfs.h @@ -235,9 +235,6 @@ #define GF_REPLICATE_TRASH_DIR ".landfill" /* GlusterFS's maximum supported Auxiliary GIDs */ -/* TODO: Keeping it to 200, so that we can fit in 2KB buffer for auth data - * in RPC server code, if there is ever need for having more aux-gids, then - * we have to add aux-gid in payload of actors */ #define GF_MAX_AUX_GROUPS 65535 #define GF_UUID_BUF_SIZE 50 diff --git a/libglusterfs/src/stack.h b/libglusterfs/src/stack.h index 20fbdfabff5..eb5848e92aa 100644 --- a/libglusterfs/src/stack.h +++ b/libglusterfs/src/stack.h @@ -357,21 +357,26 @@ STACK_RESET (call_stack_t *stack) } while (0) +static void +call_stack_set_groups (call_stack_t *stack, int ngrps, gid_t *groupbuf) +{ + stack->groups = groupbuf; + stack->ngrps = ngrps; +} + static inline int call_stack_alloc_groups (call_stack_t *stack, int ngrps) { if (ngrps <= SMALL_GROUP_COUNT) { - stack->groups = stack->groups_small; + call_stack_set_groups (stack, ngrps, stack->groups_small); } else { - stack->groups_large = GF_CALLOC (sizeof (gid_t), ngrps, + stack->groups_large = GF_CALLOC (ngrps, sizeof (gid_t), gf_common_mt_groups_t); if (!stack->groups_large) return -1; - stack->groups = stack->groups_large; + call_stack_set_groups (stack, ngrps, stack->groups_large); } - stack->ngrps = ngrps; - return 0; } -- cgit