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?= pbrezina@redhat.com 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?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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?= pbrezina@redhat.com 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.
On Tue, Nov 12, 2013 at 01:42:32PM +0100, Pavel Březina wrote:
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?= pbrezina@redhat.com 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.
There is no change in first patch related to this ^^^ comment.
bye, Sumit
} 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.
On 11/13/2013 11:43 AM, Sumit Bose wrote:
On Tue, Nov 12, 2013 at 01:42:32PM +0100, Pavel Březina wrote:
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?= pbrezina@redhat.com 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.
There is no change in first patch related to this ^^^ comment.
Sorry, I overlooked it. It turned out that it is even more tricky, since msid_str is inserted into a hash table that is returned via output parameter so it should not be freed at all.
I stealed it on sid_table instead of freeing it now.
bye, Sumit
} 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Nov 15, 2013 at 01:45:15PM +0100, Pavel Březina wrote:
On 11/13/2013 11:43 AM, Sumit Bose wrote:
On Tue, Nov 12, 2013 at 01:42:32PM +0100, Pavel Březina wrote:
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?= pbrezina@redhat.com 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.
There is no change in first patch related to this ^^^ comment.
Sorry, I overlooked it. It turned out that it is even more tricky, since msid_str is inserted into a hash table that is returned via output parameter so it should not be freed at all.
I stealed it on sid_table instead of freeing it now.
It is used as a key and hash_enter() creates an internal copy of the key. So freeing it after calling hash_enter() is safe.
bye, Sumit
bye, Sumit
} 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 11/19/2013 10:38 AM, Sumit Bose wrote:
On Fri, Nov 15, 2013 at 01:45:15PM +0100, Pavel Březina wrote:
On 11/13/2013 11:43 AM, Sumit Bose wrote:
On Tue, Nov 12, 2013 at 01:42:32PM +0100, Pavel Březina wrote:
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?= pbrezina@redhat.com 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.
There is no change in first patch related to this ^^^ comment.
Sorry, I overlooked it. It turned out that it is even more tricky, since msid_str is inserted into a hash table that is returned via output parameter so it should not be freed at all.
I stealed it on sid_table instead of freeing it now.
It is used as a key and hash_enter() creates an internal copy of the key. So freeing it after calling hash_enter() is safe.
You are right of course. Thanks. New patches are attached.
bye, Sumit
bye, Sumit
} 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Nov 20, 2013 at 02:11:42PM +0100, Pavel Březina wrote:
On 11/19/2013 10:38 AM, Sumit Bose wrote:
On Fri, Nov 15, 2013 at 01:45:15PM +0100, Pavel Březina wrote:
On 11/13/2013 11:43 AM, Sumit Bose wrote:
On Tue, Nov 12, 2013 at 01:42:32PM +0100, Pavel Březina wrote:
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?= pbrezina@redhat.com >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.
There is no change in first patch related to this ^^^ comment.
Sorry, I overlooked it. It turned out that it is even more tricky, since msid_str is inserted into a hash table that is returned via output parameter so it should not be freed at all.
I stealed it on sid_table instead of freeing it now.
It is used as a key and hash_enter() creates an internal copy of the key. So freeing it after calling hash_enter() is safe.
You are right of course. Thanks. New patches are attached.
I think this is it. Thank you for your patience. ACK
bye, Sumit
bye, Sumit
bye, Sumit
> } > > 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Nov 21, 2013 at 09:47:29PM +0100, Sumit Bose wrote:
On Wed, Nov 20, 2013 at 02:11:42PM +0100, Pavel Březina wrote:
On 11/19/2013 10:38 AM, Sumit Bose wrote:
On Fri, Nov 15, 2013 at 01:45:15PM +0100, Pavel Březina wrote:
On 11/13/2013 11:43 AM, Sumit Bose wrote:
On Tue, Nov 12, 2013 at 01:42:32PM +0100, Pavel Březina wrote:
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?= pbrezina@redhat.com >>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.
There is no change in first patch related to this ^^^ comment.
Sorry, I overlooked it. It turned out that it is even more tricky, since msid_str is inserted into a hash table that is returned via output parameter so it should not be freed at all.
I stealed it on sid_table instead of freeing it now.
It is used as a key and hash_enter() creates an internal copy of the key. So freeing it after calling hash_enter() is safe.
You are right of course. Thanks. New patches are attached.
I think this is it. Thank you for your patience. ACK
Pushed both to master.
On 11/22/2013 04:00 PM, Jakub Hrozek wrote:
On Fri, Nov 22, 2013 at 03:33:50PM +0100, Jakub Hrozek wrote:
I think this is it. Thank you for your patience. ACK
Pushed both to master.
Can you also resend the patches for 1.11 ? These don't apply cleanly.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sure.
On Mon, Nov 25, 2013 at 10:42:29AM +0100, Pavel Březina wrote:
On 11/22/2013 04:00 PM, Jakub Hrozek wrote:
On Fri, Nov 22, 2013 at 03:33:50PM +0100, Jakub Hrozek wrote:
I think this is it. Thank you for your patience. ACK
Pushed both to master.
Can you also resend the patches for 1.11 ? These don't apply cleanly.. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sure.
Ack and pushed to sssd-1-11
On Fri, Nov 15, 2013 at 01:45:15PM +0100, Pavel Březina wrote:
From b52ccbd3cdd40621d5c2300a465a4086cc2b9f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 11 Nov 2013 12:47:53 +0100 Subject: [PATCH 1/2] pac: fix double free
This patch doesn't apply anymore.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Sumit Bose