[SSSD] [PATCH] pac: fix double free

Pavel Březina pbrezina at redhat.com
Fri Nov 15 12:45:15 UTC 2013


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pac-fix-double-free.patch
Type: text/x-patch
Size: 2634 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131115/d6e9748c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-pac-fix-potential-memory-leaks.patch
Type: text/x-patch
Size: 2465 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131115/d6e9748c/attachment-0001.bin>


More information about the sssd-devel mailing list