[SSSD] [PATCH] pac: fix double free

Sumit Bose sbose at redhat.com
Tue Nov 19 09:38:15 UTC 2013


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?= <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.

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.

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



More information about the sssd-devel mailing list