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?= <[email protected]>
> >>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.
>
>
>
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
