URL: https://github.com/SSSD/sssd/pull/737
Author: alexey-tikhonov
Title: #737: providers/proxy: fixed issue 3931
Action: opened
PR body:
"""
Set of patches related with https://pagure.io/SSSD/sssd/issue/3931
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/737/head:pr737
git checkout pr737
From 68e19fa3a0e4f873931954bb8f56a9e3bf9ab4ba Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Mon, 28 Jan 2019 17:50:17 +0100
Subject: [PATCH 1/6] providers/proxy: small optimization
Small optimization of for-loops in
proxy_id.c:remove_duplicate_group_members()
---
src/providers/proxy/proxy_id.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index e82e603365..478709f693 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -602,9 +602,9 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
goto done;
}
- for (i=0; orig_grp->gr_mem[i] != NULL; i++) {
- orig_member_count++;
- }
+ for (i=0; orig_grp->gr_mem[i] != NULL; ++i) /* no-op: just counting */;
+
+ orig_member_count = i;
if (orig_member_count == 0) {
ret = ENOENT;
@@ -618,7 +618,7 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
goto done;
}
- for (i=0; orig_grp->gr_mem[i] != NULL; i++) {
+ for (i=0; i < orig_member_count; ++i) {
key.type = HASH_KEY_STRING;
key.str = talloc_strdup(member_tbl, orig_grp->gr_mem[i]);
if (key.str == NULL) {
From d8b8098ac137add9f00f52fe2a0347cf53d7a074 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Mon, 28 Jan 2019 18:30:21 +0100
Subject: [PATCH 2/6] providers/proxy: fixed wrong check
Fixed evident "copy-paste" bug with wrong var being checked for NULL
in proxy_id.c:remove_duplicate_group_members()
---
src/providers/proxy/proxy_id.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 478709f693..db65a984cd 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -629,7 +629,7 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
value.type = HASH_VALUE_PTR;
value.ptr = talloc_strdup(member_tbl, orig_grp->gr_mem[i]);
- if (key.str == NULL) {
+ if (value.ptr == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
ret = ENOMEM;
goto done;
From e0de9be7371c942f2b4ba1398a7790207c0b958b Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Mon, 28 Jan 2019 18:47:27 +0100
Subject: [PATCH 3/6] providers/proxy: fixed usage of wrong mem ctx
Temporary var `grp` in proxy_id.c:remove_duplicate_group_members()
should be created in `tmp_ctx`.
Call to
```
*_grp = talloc_steal(mem_ctx, grp);
```
as well confirms it was original intent
(before fix this call didn't have any sense).
Having `grp` created in `mem_ctx` may lead to memory leak in case
of failure. While actually this doesn't happen since caller of
remove_duplicate_group_members() cleans mem_ctx, still it is
good to fix it.
---
src/providers/proxy/proxy_id.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index db65a984cd..52f7a64243 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -649,7 +649,7 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
goto done;
}
- grp = talloc(mem_ctx, struct group);
+ grp = talloc(tmp_ctx, struct group);
if (grp == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc failed.\n");
ret = ENOMEM;
From ce7c1184ddb4a0779e35068e840631772d4884fc Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Mon, 28 Jan 2019 19:23:46 +0100
Subject: [PATCH 4/6] providers/proxy: got rid of excessive mem copies
There is no need to create copies of strings for temporary storage
in hash_table.
---
src/providers/proxy/proxy_id.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 52f7a64243..3e8a43ad7c 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -620,24 +620,15 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
for (i=0; i < orig_member_count; ++i) {
key.type = HASH_KEY_STRING;
- key.str = talloc_strdup(member_tbl, orig_grp->gr_mem[i]);
- if (key.str == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
- ret = ENOMEM;
- goto done;
- }
+ key.str = orig_grp->gr_mem[i]; /* hash_enter() makes copy itself */
value.type = HASH_VALUE_PTR;
- value.ptr = talloc_strdup(member_tbl, orig_grp->gr_mem[i]);
- if (value.ptr == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
- ret = ENOMEM;
- goto done;
- }
+ /* no need to put copy in hash_table since
+ copy will be created during construction of new grp */
+ value.ptr = orig_grp->gr_mem[i];
ret = hash_enter(member_tbl, &key, &value);
if (ret != HASH_SUCCESS) {
- talloc_free(key.str);
ret = ENOMEM;
goto done;
}
From a8f069a8f56c89e44468e7b13f4bcdf2d3da35c0 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Tue, 29 Jan 2019 12:13:30 +0100
Subject: [PATCH 5/6] providers/proxy: fixed erroneous free of orig_grp
Function `remove_duplicate_group_members(mem_ctx, orig_grp, new_grp)`
in case of empty orig_grp would return as a result:
```
*new_grp = talloc_steal(mem_ctx, orig_grp);
```
Since mem_ctx is freed in caller function that leads to deallocation
of orig_grp and to "use after free" bug.
Code was changes so remove_duplicate_group_members() behaves consistently
and always returns a new group created in given mem context.
Resolves: https://pagure.io/SSSD/sssd/issue/3931
---
src/providers/proxy/proxy_id.c | 66 ++++++++++++++++++----------------
1 file changed, 35 insertions(+), 31 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 3e8a43ad7c..3c754f079d 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -597,8 +597,32 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
return ENOMEM;
}
+ grp = talloc(tmp_ctx, struct group);
+ if (grp == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc failed.\n");
+ ret = ENOMEM;
+ goto done;
+ }
+
+ grp->gr_gid = orig_grp->gr_gid;
+
+ grp->gr_name = talloc_strdup(grp, orig_grp->gr_name);
+ if (grp->gr_name == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
+ ret = ENOMEM;
+ goto done;
+ }
+
+ grp->gr_passwd = talloc_strdup(grp, orig_grp->gr_passwd);
+ if (grp->gr_passwd == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
+ ret = ENOMEM;
+ goto done;
+ }
+
if (orig_grp->gr_mem == NULL) {
- ret = ENOENT;
+ grp->gr_mem = NULL;
+ ret = EOK;
goto done;
}
@@ -607,7 +631,14 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
orig_member_count = i;
if (orig_member_count == 0) {
- ret = ENOENT;
+ grp->gr_mem = talloc_zero_array(grp, char *, 1);
+ if (grp->gr_mem == NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "talloc_zero_array failed.\n");
+ ret = ENOMEM;
+ goto done;
+ }
+ grp->gr_mem[0] = NULL;
+ ret = EOK;
goto done;
}
@@ -636,14 +667,8 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
member_count = hash_count(member_tbl);
if (member_count == 0) {
- ret = ENOENT;
- goto done;
- }
-
- grp = talloc(tmp_ctx, struct group);
- if (grp == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "talloc failed.\n");
- ret = ENOMEM;
+ DEBUG(SSSDBG_CRIT_FAILURE, "Empty resulting hash table - must be internal bug.\n");
+ ret = EINVAL;
goto done;
}
@@ -673,33 +698,12 @@ static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
}
grp->gr_mem[i] = NULL;
- grp->gr_gid = orig_grp->gr_gid;
-
- grp->gr_name = talloc_strdup(grp, orig_grp->gr_name);
- if (grp->gr_name == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
- ret = ENOMEM;
- goto done;
- }
-
- grp->gr_passwd = talloc_strdup(grp, orig_grp->gr_passwd);
- if (grp->gr_passwd == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
- ret = ENOMEM;
- goto done;
- }
-
*_grp = talloc_steal(mem_ctx, grp);
ret = EOK;
done:
talloc_zfree(tmp_ctx);
- if (ret == ENOENT) {
- *_grp = talloc_steal(mem_ctx, orig_grp);
- ret = EOK;
- }
-
return ret;
}
From 6c2706e53cdabdf157568decbfab7ce45113569c Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikh...@redhat.com>
Date: Tue, 29 Jan 2019 12:36:36 +0100
Subject: [PATCH 6/6] providers/proxy: const params should be const
grp/orig_grp param of save_group() and remove_duplicate_group_members()
is not intended to be changed so marked as pointer to const data.
---
src/providers/proxy/proxy_id.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index 3c754f079d..e1be29076a 100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -576,7 +576,7 @@ static int enum_users(TALLOC_CTX *mem_ctx,
static errno_t remove_duplicate_group_members(TALLOC_CTX *mem_ctx,
- struct group *orig_grp,
+ const struct group *orig_grp,
struct group **_grp)
{
TALLOC_CTX *tmp_ctx;
@@ -713,7 +713,7 @@ static errno_t proxy_process_missing_users(struct sysdb_ctx *sysdb,
const char *const*fq_gr_mem,
time_t now);
static int save_group(struct sysdb_ctx *sysdb, struct sss_domain_info *dom,
- struct group *grp,
+ const struct group *grp,
const char *real_name, /* already qualified */
const char *alias) /* already qualified */
{
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
