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