On Thu, Nov 21, 2013 at 09:47:29PM +0100, Sumit Bose wrote: > 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?= <[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. > > >> > > >>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
Pushed both to master. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
