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