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(a)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(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>