From a413f8754a7389c1b5dd93c219b47b9f904711fb Mon Sep 17 00:00:00 2001
From: Csaba Henk <csaba@redhat.com>
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 <csaba@redhat.com>
> Reviewed-on: https://review.gluster.org/18789
Change-Id: I61f4574ba21969f7661b9ff0c9dce202b874025d
BUG: 1527147
Signed-off-by: Csaba Henk <csaba@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/126437
Reviewed-by: Csaba Henk <chenk@redhat.com>
Tested-by: Csaba Henk <chenk@redhat.com>
Tested-by: RHGS Build Bot <nigelb@redhat.com>
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
---
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