On 11/12/2013 10:03 AM, Sumit Bose wrote:
On Mon, Nov 11, 2013 at 12:50:54PM +0100, Pavel Březina wrote:
From f9f6e1ce452f9dc507c4779e6ff74aea412e9457 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
Date: Mon, 11 Nov 2013 12:47:53 +0100
Subject: [PATCH] pac: fix double free
---
src/responder/pac/pacsrv_utils.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/responder/pac/pacsrv_utils.c b/src/responder/pac/pacsrv_utils.c
index
30055a1345b7d943e6adf822438263c92e53b51a..8380247b10c74d8acabec8e1a8c22944aab38320
100644
--- a/src/responder/pac/pacsrv_utils.c
+++ b/src/responder/pac/pacsrv_utils.c
@@ -74,6 +74,7 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
struct sss_domain_info *user_dom;
struct sss_domain_info *group_dom;
char *sid_str = NULL;
+ char *msid_str = NULL;
char *user_dom_sid_str = NULL;
size_t user_dom_sid_str_len;
enum idmap_error_code err;
@@ -231,24 +232,22 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
}
- talloc_zfree(sid_str);
-
for(s = 0; s < info3->sidcount; s++) {
err = sss_idmap_smb_sid_to_sid(pac_ctx->idmap_ctx, info3->sids[s].sid,
- &sid_str);
+ &msid_str);
if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_OP_FAILURE, ("sss_idmap_smb_sid_to_sid failed.\n"));
ret = EFAULT;
goto done;
}
- key.str = sid_str;
+ key.str = msid_str;
value.ul = 0;
- ret = responder_get_domain_by_id(pac_ctx->rctx, sid_str, &group_dom);
+ ret = responder_get_domain_by_id(pac_ctx->rctx, msid_str, &group_dom);
if (ret == EOK) {
ret = sysdb_search_object_by_sid(mem_ctx, group_dom->sysdb,
- group_dom, sid_str, NULL, &msg);
+ group_dom, msid_str, NULL, &msg);
if (ret == EOK && msg->count == 1 ) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0],
SYSDB_GIDNUM, 0);
@@ -263,14 +262,13 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
ret = EIO;
goto done;
}
-
- sss_idmap_free_sid(pac_ctx->idmap_ctx, sid_str);
I think this should be replaced by
sss_idmap_free_sid(pac_ctx->idmap_ctx, msid_str);
msid_str = NULL;
otherwise only the last allocated msid_str will be freed in the done
section.
}
ret = EOK;
done:
talloc_free(sid_str);
+ sss_idmap_free_sid(pac_ctx->idmap_ctx, msid_str);
sss_idmap_free_sid(pac_ctx->idmap_ctx, user_dom_sid_str);
if (ret == EOK) {
--
1.7.11.7
While looking at the code I think there is a potential memory leak in
this section as well:
ret = responder_get_domain_by_id(pac_ctx->rctx, sid_str, &group_dom);
if (ret == EOK) {
ret = sysdb_search_object_by_sid(mem_ctx, group_dom->sysdb,
group_dom, sid_str, NULL, &msg);
if (ret == EOK && msg->count == 1 ) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0],
SYSDB_GIDNUM, 0);
talloc_free(msg);
}
}
while msg should not be set if ret != EOK it will certainly be set if
msg->count != 1. Although it should never happen would you mind to
remove the talloc_free(msg) out of the if block and add a
talloc_zfree(msg) after the if block?
Nice catch. Patches are attached.
From 81e79170f3f2abb6ce1f736ffca9b3a338226659 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
Date: Mon, 11 Nov 2013 12:47:53 +0100
Subject: [PATCH 1/2] pac: fix double free
---
src/responder/pac/pacsrv_utils.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/responder/pac/pacsrv_utils.c b/src/responder/pac/pacsrv_utils.c
index 30055a1345b7d943e6adf822438263c92e53b51a..8380247b10c74d8acabec8e1a8c22944aab38320 100644
--- a/src/responder/pac/pacsrv_utils.c
+++ b/src/responder/pac/pacsrv_utils.c
@@ -74,6 +74,7 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
struct sss_domain_info *user_dom;
struct sss_domain_info *group_dom;
char *sid_str = NULL;
+ char *msid_str = NULL;
char *user_dom_sid_str = NULL;
size_t user_dom_sid_str_len;
enum idmap_error_code err;
@@ -231,24 +232,22 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
}
- talloc_zfree(sid_str);
-
for(s = 0; s < info3->sidcount; s++) {
err = sss_idmap_smb_sid_to_sid(pac_ctx->idmap_ctx, info3->sids[s].sid,
- &sid_str);
+ &msid_str);
if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_OP_FAILURE, ("sss_idmap_smb_sid_to_sid failed.\n"));
ret = EFAULT;
goto done;
}
- key.str = sid_str;
+ key.str = msid_str;
value.ul = 0;
- ret = responder_get_domain_by_id(pac_ctx->rctx, sid_str, &group_dom);
+ ret = responder_get_domain_by_id(pac_ctx->rctx, msid_str, &group_dom);
if (ret == EOK) {
ret = sysdb_search_object_by_sid(mem_ctx, group_dom->sysdb,
- group_dom, sid_str, NULL, &msg);
+ group_dom, msid_str, NULL, &msg);
if (ret == EOK && msg->count == 1 ) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0],
SYSDB_GIDNUM, 0);
@@ -263,14 +262,13 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
ret = EIO;
goto done;
}
-
- sss_idmap_free_sid(pac_ctx->idmap_ctx, sid_str);
}
ret = EOK;
done:
talloc_free(sid_str);
+ sss_idmap_free_sid(pac_ctx->idmap_ctx, msid_str);
sss_idmap_free_sid(pac_ctx->idmap_ctx, user_dom_sid_str);
if (ret == EOK) {
--
1.7.11.7
From 1055cac846d992ce85cae3b489af4253d46850df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
Date: Tue, 12 Nov 2013 13:41:49 +0100
Subject: [PATCH 2/2] pac: fix potential memory leaks
---
src/responder/pac/pacsrv_utils.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/responder/pac/pacsrv_utils.c b/src/responder/pac/pacsrv_utils.c
index 8380247b10c74d8acabec8e1a8c22944aab38320..336a8cda064db411c0a6b0cf9924ee803b07b506 100644
--- a/src/responder/pac/pacsrv_utils.c
+++ b/src/responder/pac/pacsrv_utils.c
@@ -82,7 +82,7 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
hash_key_t key;
hash_value_t value;
char *rid_start;
- struct ldb_result *msg;
+ struct ldb_result *msg = NULL;
char *user_sid_str = NULL;
char *primary_group_sid_str = NULL;
@@ -154,8 +154,8 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
sid_str, NULL, &msg);
if (ret == EOK && msg->count == 1) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0], SYSDB_UIDNUM, 0);
- talloc_free(msg);
}
+ talloc_zfree(msg);
ret = hash_enter(sid_table, &key, &value);
if (ret != HASH_SUCCESS) {
@@ -189,8 +189,8 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
sid_str, NULL, &msg);
if (ret == EOK && msg->count == 1) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0], SYSDB_GIDNUM, 0);
- talloc_free(msg);
}
+ talloc_zfree(msg);
ret = hash_enter(sid_table, &key, &value);
if (ret != HASH_SUCCESS) {
@@ -219,8 +219,8 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
if (ret == EOK && msg->count == 1) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0],
SYSDB_GIDNUM, 0);
- talloc_free(msg);
}
+ talloc_zfree(msg);
ret = hash_enter(sid_table, &key, &value);
if (ret != HASH_SUCCESS) {
@@ -251,8 +251,8 @@ errno_t get_sids_from_pac(TALLOC_CTX *mem_ctx,
if (ret == EOK && msg->count == 1 ) {
value.ul = ldb_msg_find_attr_as_uint64(msg->msgs[0],
SYSDB_GIDNUM, 0);
- talloc_free(msg);
}
+ talloc_zfree(msg);
}
ret = hash_enter(sid_table, &key, &value);
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
