From a413f8754a7389c1b5dd93c219b47b9f904711fb Mon Sep 17 00:00:00 2001 From: Csaba Henk Date: Fri, 15 Dec 2017 08:02:30 +0100 Subject: [PATCH 644/644] libglusterfs: fix the call_stack_set_group() function - call_stack_set_group() will take the ownership of passed buffer from caller; - to indicate the change, its signature is changed from including the buffer directly to take a pointer to it; - either the content of the buffer is copied to the groups_small embedded buffer of the call stack, or the buffer is set as groups_large member of the call stack; - the groups member of the call stack is set to, respectively, groups_small or groups_large, according to the memory management conventions of the call stack; - the buffer address is overwritten with junk to effectively prevent the caller from using it further on. Also move call_stack_set_group to stack.c from stack.h to prevent "defined but not used [-Wunused-function]" warnings (not using it anymore in call_stack_alloc_group() implementation, which saved us from this so far). protocol/server: refactor gid_resolve() In gid_resolve there are two cases: either the gid_cache_lookup() call returns a value or not. The result is caputured in the agl variable, and throughout the function, each particular stage of the implementation comes with an agl and a no-agl variant. In most cases this is explicitly indicated via an if (agl) { ... } else { ... } but some of this branching are expressed via goto constructs (obfuscating the fact we stated above, that is, each particular stage having an agl/no-agl variant). In the current refactor, we bring the agl conditional to the top, and present the agl/non-agl implementations sequentially. Also we take the opportunity to clean up and fix the agl case: - remove the spurious gl.gl_list = agl->gl_list; setting, as gl is not used in the agl caae - populate the group list of call stack from agl, fixing thus referred BUG. > Change-Id: I61f4574ba21969f7661b9ff0c9dce202b874025d > BUG: 1513928 > Signed-off-by: Csaba Henk > Reviewed-on: https://review.gluster.org/18789 Change-Id: I61f4574ba21969f7661b9ff0c9dce202b874025d BUG: 1527147 Signed-off-by: Csaba Henk Reviewed-on: https://code.engineering.redhat.com/gerrit/126437 Reviewed-by: Csaba Henk Tested-by: Csaba Henk Tested-by: RHGS Build Bot Reviewed-by: Atin Mukherjee --- libglusterfs/src/stack.c | 20 +++++++++ libglusterfs/src/stack.h | 14 +++--- xlators/mount/fuse/src/fuse-helpers.c | 2 +- xlators/protocol/server/src/server-helpers.c | 65 +++++++++++++--------------- 4 files changed, 57 insertions(+), 44 deletions(-) diff --git a/libglusterfs/src/stack.c b/libglusterfs/src/stack.c index 6977814..d64ac8a 100644 --- a/libglusterfs/src/stack.c +++ b/libglusterfs/src/stack.c @@ -65,6 +65,26 @@ create_frame (xlator_t *xl, call_pool_t *pool) } void +call_stack_set_groups (call_stack_t *stack, int ngrps, gid_t **groupbuf_p) +{ + /* We take the ownership of the passed group buffer. */ + + if (ngrps <= SMALL_GROUP_COUNT) { + memcpy (stack->groups_small, *groupbuf_p, + sizeof (gid_t) * ngrps); + stack->groups = stack->groups_small; + GF_FREE (*groupbuf_p); + } else { + stack->groups_large = *groupbuf_p; + stack->groups = stack->groups_large; + } + + stack->ngrps = ngrps; + /* Set a canary. */ + *groupbuf_p = (void *)0xdeadf00d; +} + +void gf_proc_dump_call_frame (call_frame_t *call_frame, const char *key_buf,...) { diff --git a/libglusterfs/src/stack.h b/libglusterfs/src/stack.h index c2848b7..a71a150 100644 --- a/libglusterfs/src/stack.h +++ b/libglusterfs/src/stack.h @@ -422,26 +422,21 @@ 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) { - call_stack_set_groups (stack, ngrps, stack->groups_small); + stack->groups = stack->groups_small; } else { stack->groups_large = GF_CALLOC (ngrps, sizeof (gid_t), gf_common_mt_groups_t); if (!stack->groups_large) return -1; - call_stack_set_groups (stack, ngrps, stack->groups_large); + stack->groups = stack->groups_large; } + stack->ngrps = ngrps; + return 0; } @@ -530,6 +525,7 @@ copy_frame (call_frame_t *frame) return newframe; } +void call_stack_set_groups (call_stack_t *stack, int ngrps, gid_t **groupbuf_p); void gf_proc_dump_pending_frames(call_pool_t *call_pool); void gf_proc_dump_pending_frames_to_dict (call_pool_t *call_pool, dict_t *dict); diff --git a/xlators/mount/fuse/src/fuse-helpers.c b/xlators/mount/fuse/src/fuse-helpers.c index 5ccb0a5..cd43ded 100644 --- a/xlators/mount/fuse/src/fuse-helpers.c +++ b/xlators/mount/fuse/src/fuse-helpers.c @@ -181,7 +181,7 @@ frame_fill_groups (call_frame_t *frame) return; } - call_stack_set_groups (frame->root, ngroups, mygroups); + call_stack_set_groups (frame->root, ngroups, &mygroups); } else { ret = snprintf (filename, sizeof filename, "/proc/%d/status", frame->root->pid); diff --git a/xlators/protocol/server/src/server-helpers.c b/xlators/protocol/server/src/server-helpers.c index 647f144..a01e72f 100644 --- a/xlators/protocol/server/src/server-helpers.c +++ b/xlators/protocol/server/src/server-helpers.c @@ -31,13 +31,24 @@ gid_resolve (server_conf_t *conf, call_stack_t *root) struct passwd *result; gid_t *mygroups; gid_list_t gl; - const gid_list_t *agl; int ngroups; + const gid_list_t *agl; agl = gid_cache_lookup (&conf->gid_cache, root->uid, 0, 0); if (agl) { root->ngrps = agl->gl_count; - goto fill_groups; + + if (root->ngrps > 0) { + ret = call_stack_alloc_groups (root, agl->gl_count); + if (ret == 0) { + memcpy (root->groups, agl->gl_list, + sizeof (gid_t) * agl->gl_count); + } + } + + gid_cache_release (&conf->gid_cache, agl); + + return ret; } ret = getpwuid_r (root->uid, &mypw, mystrs, sizeof(mystrs), &result); @@ -66,42 +77,28 @@ gid_resolve (server_conf_t *conf, call_stack_t *root) } root->ngrps = (uint16_t) ngroups; -fill_groups: - if (agl) { - /* the gl is not complete, we only use gl.gl_list later on */ - gl.gl_list = agl->gl_list; - } else { - /* setup a full gid_list_t to add it to the gid_cache */ - gl.gl_id = root->uid; - gl.gl_uid = root->uid; - gl.gl_gid = root->gid; - gl.gl_count = root->ngrps; - - gl.gl_list = GF_MALLOC (root->ngrps * sizeof(gid_t), - gf_common_mt_groups_t); - if (gl.gl_list) - memcpy (gl.gl_list, mygroups, - sizeof(gid_t) * root->ngrps); - else { - GF_FREE (mygroups); - return -1; - } + /* setup a full gid_list_t to add it to the gid_cache */ + gl.gl_id = root->uid; + gl.gl_uid = root->uid; + gl.gl_gid = root->gid; + gl.gl_count = root->ngrps; + + gl.gl_list = GF_MALLOC (root->ngrps * sizeof(gid_t), + gf_common_mt_groups_t); + if (gl.gl_list) + memcpy (gl.gl_list, mygroups, + sizeof(gid_t) * root->ngrps); + else { + GF_FREE (mygroups); + return -1; } - if (root->ngrps == 0) { - ret = 0; - goto out; + if (root->ngrps > 0) { + call_stack_set_groups (root, root->ngrps, &mygroups); } - call_stack_set_groups (root, root->ngrps, mygroups); - -out: - if (agl) { - gid_cache_release (&conf->gid_cache, agl); - } else { - if (gid_cache_add (&conf->gid_cache, &gl) != 1) - GF_FREE (gl.gl_list); - } + if (gid_cache_add (&conf->gid_cache, &gl) != 1) + GF_FREE (gl.gl_list); return ret; } -- 1.8.3.1