[SSSD] [PATCH] pac: fix double free
Pavel Březina
pbrezina at redhat.com
Tue Nov 12 12:42:32 UTC 2013
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.
>
>> }
>>
>> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pac-fix-double-free.patch
Type: text/x-patch
Size: 2494 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131112/f7e98231/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/20131112/f7e98231/attachment-0001.bin>
More information about the sssd-devel
mailing list