From ba4af70fb7d3ae21187ac0e9e3fe5539a4f8f829 Mon Sep 17 00:00:00 2001 From: Csaba Henk Date: Wed, 5 Jul 2017 17:48:37 +0200 Subject: [PATCH 545/557] 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 > 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 BUG: 1452513 Change-Id: Icea76d0d74dcf2f87d26cb299acc771ca3b32d2b Signed-off-by: Csaba Henk Reviewed-on: https://code.engineering.redhat.com/gerrit/111305 Tested-by: Csaba Henk Reviewed-by: Atin Mukherjee --- libglusterfs/src/common-utils.c | 73 ++++++++++++++++++++++++++++ libglusterfs/src/common-utils.h | 3 ++ libglusterfs/src/glusterfs.h | 3 -- libglusterfs/src/stack.h | 15 ++++-- xlators/mount/fuse/src/fuse-helpers.c | 20 +++----- xlators/nfs/server/src/nfs-fops.c | 36 +++++--------- xlators/protocol/server/src/server-helpers.c | 23 +++------ 7 files changed, 113 insertions(+), 60 deletions(-) diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c index 862ec3b..c123f70 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 @@ -4853,3 +4854,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 1af9529..4765b5e 100644 --- a/libglusterfs/src/common-utils.h +++ b/libglusterfs/src/common-utils.h @@ -903,4 +903,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 d812aa7..43acf4c 100644 --- a/libglusterfs/src/glusterfs.h +++ b/libglusterfs/src/glusterfs.h @@ -233,9 +233,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 be7e900..c2848b7 100644 --- a/libglusterfs/src/stack.h +++ b/libglusterfs/src/stack.h @@ -422,21 +422,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; } diff --git a/xlators/mount/fuse/src/fuse-helpers.c b/xlators/mount/fuse/src/fuse-helpers.c index 3e54197..5ccb0a5 100644 --- a/xlators/mount/fuse/src/fuse-helpers.c +++ b/xlators/mount/fuse/src/fuse-helpers.c @@ -20,7 +20,6 @@ #include #endif #include -#include #include "fuse-bridge.h" @@ -158,8 +157,8 @@ frame_fill_groups (call_frame_t *frame) char *saveptr = NULL; char *endptr = NULL; int ret = 0; - int ngroups = FUSE_MAX_AUX_GROUPS; - gid_t mygroups[GF_MAX_AUX_GROUPS]; + int ngroups = 0; + gid_t *mygroups = NULL; if (priv->resolve_gids) { struct passwd pwent; @@ -173,23 +172,16 @@ frame_fill_groups (call_frame_t *frame) return; } - ngroups = GF_MAX_AUX_GROUPS; - if (getgrouplist (result->pw_name, frame->root->gid, mygroups, - &ngroups) == -1) { + ngroups = gf_getgrouplist (result->pw_name, frame->root->gid, + &mygroups); + if (ngroups == -1) { gf_log (this->name, GF_LOG_ERROR, "could not map %s to " "group list (ngroups %d, max %d)", result->pw_name, ngroups, GF_MAX_AUX_GROUPS); return; } - if (call_stack_alloc_groups (frame->root, ngroups) != 0) - goto out; - - /* Copy data to the frame. */ - for (idx = 0; idx < ngroups; ++idx) { - frame->root->groups[idx] = mygroups[idx]; - } - frame->root->ngrps = ngroups; + call_stack_set_groups (frame->root, ngroups, mygroups); } else { ret = snprintf (filename, sizeof filename, "/proc/%d/status", frame->root->pid); diff --git a/xlators/nfs/server/src/nfs-fops.c b/xlators/nfs/server/src/nfs-fops.c index f6361f0..28b1781 100644 --- a/xlators/nfs/server/src/nfs-fops.c +++ b/xlators/nfs/server/src/nfs-fops.c @@ -8,7 +8,6 @@ cases as published by the Free Software Foundation. */ -#include #include #include "dict.h" @@ -34,12 +33,7 @@ nfs_fix_groups (xlator_t *this, call_stack_t *root) struct passwd mypw; char mystrs[1024]; struct passwd *result; -#ifdef GF_DARWIN_HOST_OS - /* BSD/DARWIN does not correctly uses gid_t in getgrouplist */ - int mygroups[GF_MAX_AUX_GROUPS]; -#else - gid_t mygroups[GF_MAX_AUX_GROUPS]; -#endif + gid_t *mygroups; int ngroups; int i; int max_groups; @@ -88,25 +82,13 @@ nfs_fix_groups (xlator_t *this, call_stack_t *root) gf_msg_trace (this->name, 0, "mapped %u => %s", root->uid, result->pw_name); - ngroups = GF_MAX_AUX_GROUPS; - if (getgrouplist(result->pw_name,root->gid,mygroups,&ngroups) == -1) { + ngroups = gf_getgrouplist (result->pw_name, root->gid, &mygroups); + if (ngroups == -1) { gf_msg (this->name, GF_LOG_ERROR, 0, NFS_MSG_MAP_GRP_LIST_FAIL, "could not map %s to group list", result->pw_name); return; } - /* Add the group data to the cache. */ - gl.gl_list = GF_CALLOC(ngroups, sizeof(gid_t), gf_nfs_mt_aux_gids); - if (gl.gl_list) { - /* It's not fatal if the alloc failed. */ - gl.gl_id = root->uid; - gl.gl_uid = 0; - gl.gl_gid = 0; - gl.gl_count = ngroups; - memcpy(gl.gl_list, mygroups, sizeof(gid_t) * ngroups); - if (gid_cache_add(&priv->gid_cache, &gl) != 1) - GF_FREE(gl.gl_list); - } /* RPC enforces the GF_AUTH_GLUSTERFS_MAX_GROUPS limit */ if (ngroups > max_groups) { @@ -115,16 +97,24 @@ nfs_fix_groups (xlator_t *this, call_stack_t *root) "too many groups, reducing %d -> %d", ngroups, max_groups); - ngroups = max_groups; } /* Copy data to the frame. */ - for (i = 0; i < ngroups; ++i) { + for (i = 0; i < ngroups && i < max_groups; ++i) { gf_msg_trace (this->name, 0, "%s is in group %u", result->pw_name, mygroups[i]); root->groups[i] = mygroups[i]; } root->ngrps = ngroups; + + /* Add the group data to the cache. */ + gl.gl_list = mygroups; + gl.gl_id = root->uid; + gl.gl_uid = 0; + gl.gl_gid = 0; + gl.gl_count = ngroups; + if (gid_cache_add(&priv->gid_cache, &gl) != 1) + GF_FREE(mygroups); } struct nfs_fop_local * diff --git a/xlators/protocol/server/src/server-helpers.c b/xlators/protocol/server/src/server-helpers.c index 590662b..3ad3d84 100644 --- a/xlators/protocol/server/src/server-helpers.c +++ b/xlators/protocol/server/src/server-helpers.c @@ -19,7 +19,6 @@ #include #include -#include #include "compound-fop-utils.h" /* based on nfs_fix_aux_groups() */ @@ -30,10 +29,10 @@ gid_resolve (server_conf_t *conf, call_stack_t *root) struct passwd mypw; char mystrs[1024]; struct passwd *result; - gid_t mygroups[GF_MAX_AUX_GROUPS]; + gid_t *mygroups; gid_list_t gl; const gid_list_t *agl; - int ngroups, i; + int ngroups; agl = gid_cache_lookup (&conf->gid_cache, root->uid, 0, 0); if (agl) { @@ -58,9 +57,8 @@ gid_resolve (server_conf_t *conf, call_stack_t *root) gf_msg_trace ("gid-cache", 0, "mapped %u => %s", root->uid, result->pw_name); - ngroups = GF_MAX_AUX_GROUPS; - ret = getgrouplist (result->pw_name, root->gid, mygroups, &ngroups); - if (ret == -1) { + ngroups = gf_getgrouplist (result->pw_name, root->gid, &mygroups); + if (ngroups == -1) { gf_msg ("gid-cache", GF_LOG_ERROR, 0, PS_MSG_MAPPING_ERROR, "could not map %s to group list (%d gids)", result->pw_name, root->ngrps); @@ -84,8 +82,10 @@ fill_groups: if (gl.gl_list) memcpy (gl.gl_list, mygroups, sizeof(gid_t) * root->ngrps); - else + else { + GF_FREE (mygroups); return -1; + } } if (root->ngrps == 0) { @@ -93,14 +93,7 @@ fill_groups: goto out; } - if (call_stack_alloc_groups (root, root->ngrps) != 0) { - ret = -1; - goto out; - } - - /* finally fill the groups from the */ - for (i = 0; i < root->ngrps; ++i) - root->groups[i] = gl.gl_list[i]; + call_stack_set_groups (root, root->ngrps, mygroups); out: if (agl) { -- 1.8.3.1