[SSSD] [PATCH] AD: Cache gpo version; only download policy files if version changes

Lukas Slebodnik lslebodn at redhat.com
Wed Jul 23 07:21:41 UTC 2014


On (23/07/14 03:03), Yassir Elley wrote:
>
>
>----- Original Message -----
>> On Tue, Jul 22, 2014 at 03:13:58AM -0400, Yassir Elley wrote:
>> 
>> See some comments about ad_gpo.c inline, I'm still reading
>> ad_gpo_child.c
>> 
>> > From f3f00a78102ad5e389266b28c43d0396caf57592 Mon Sep 17 00:00:00 2001
>> > From: Yassir Elley <yelley at redhat.com>
>> > Date: Tue, 22 Jul 2014 03:03:03 -0400
>> > Subject: [PATCH 2/2] AD-GPO: only download policy files if gpo version
>> > changes
>> 
>> [...]
>> 
>> > @@ -1298,8 +1304,6 @@ ad_gpo_cse_step(struct tevent_req *req)
>> >  {
>> >      struct tevent_req *subreq;
>> >      struct ad_gpo_access_state *state;
>> > -    char *cse_smb_uri;
>> > -    char *cse_unix_path;
>> >      int i = 0;
>> >  
>> >      state = tevent_req_data(req, struct ad_gpo_access_state);
>> 
>> It's not really visible from the diff, but ad_gpo_cse_step increments
>> the index as the first thing it does and only then assigns to
>> cse_filtered_gpo. This reads to me as if you were skipping the first
>> entry...shouldn't the code read the other way around, first assign the
>> GPO and then increment the index?
>
>I ran into problems when using an index initialized to 0. Although I don't remember what those problems were anymore, I was able to solve them by using an index initialized to -1, which is what I am using now, and which is why the current code works correctly (and doesn't skip the first entry).
>
>> 
>> > @@ -1319,15 +1323,19 @@ ad_gpo_cse_step(struct tevent_req *req)
>> >                state->cse_gpo_index, i,
>> >                cse_filtered_gpo->gpo_cse_guids[i]);
>> >      }
>> >  
>> > -    cse_smb_uri = talloc_asprintf(state, "%s%s",
>> > -                                  cse_filtered_gpo->gpo_file_sys_path,
>> > -                                  GP_EXT_GUID_SECURITY_SUFFIX);
>> > +    DEBUG(SSSDBG_TRACE_FUNC, "smb_server: %s\n",
>> > cse_filtered_gpo->smb_server);
>> > +    DEBUG(SSSDBG_TRACE_FUNC, "smb_share: %s\n",
>> > cse_filtered_gpo->smb_share);
>> > +    DEBUG(SSSDBG_TRACE_FUNC, "smb_path: %s\n",
>> > cse_filtered_gpo->smb_path);
>> > +    DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n",
>> > cse_filtered_gpo->gpo_guid);
>> >  
>> > -    cse_unix_path = talloc_asprintf(state, "%s%s",
>> > -                                    cse_filtered_gpo->gpo_unix_path,
>> > -                                    GP_EXT_GUID_SECURITY_SUFFIX);
>> > -
>> > -    subreq = ad_gpo_process_cse_send(state, state->ev, cse_smb_uri,
>> > cse_unix_path);
>> > +    subreq = ad_gpo_process_cse_send(state,
>> > +                                     state->ev,
>> > +                                     state->domain,
>> > +                                     cse_filtered_gpo->smb_server,
>> > +                                     cse_filtered_gpo->smb_share,
>> > +                                     cse_filtered_gpo->smb_path,
>> > +                                     GP_EXT_GUID_SECURITY_SUFFIX,
>> > +                                     cse_filtered_gpo->gpo_guid);
>> >  
>> >      tevent_req_set_callback(subreq, ad_gpo_cse_done, req);
>> >      return EAGAIN;
>> > @@ -1348,6 +1356,7 @@ ad_gpo_cse_done(struct tevent_req *subreq)
>> >      struct tevent_req *req;
>> >      struct ad_gpo_access_state *state;
>> >      int ret;
>> > +    int sysvol_gpt_version;
>> >      char **allowed_sids;
>> >      int allowed_size;
>> >      char **denied_sids;
>> > @@ -1356,11 +1365,23 @@ ad_gpo_cse_done(struct tevent_req *subreq)
>> >      req = tevent_req_callback_data(subreq, struct tevent_req);
>> >      state = tevent_req_data(req, struct ad_gpo_access_state);
>> >  
>> > -    ret = ad_gpo_process_cse_recv(subreq, state, &allowed_size,
>> > +    struct gp_gpo *cse_filtered_gpo =
>> > +        state->cse_filtered_gpos[state->cse_gpo_index];
>> > +
>> > +    const char *gpo_guid = cse_filtered_gpo->gpo_guid;
>> > +
>> > +    DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n", gpo_guid);
>> > +
>> > +    ret = ad_gpo_process_cse_recv(subreq, state, &sysvol_gpt_version,
>> > &allowed_size,
>> >                                    &allowed_sids, &denied_size,
>> >                                    &denied_sids);
>> 
>> Here you read the return code of ad_gpo_process_cse_recv() but don't
>> check it.
>
>OK.
>
>> 
>> >  
>> >      talloc_zfree(subreq);
>> >  
>> > +
>> > +    DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n",
>> > sysvol_gpt_version);
>> > +
>> > +    ret = sysdb_gpo_store_gpo(state->domain, gpo_guid,
>> > sysvol_gpt_version);
>> > +
>> >      if (ret != EOK) {
>> >          /* TBD: handle ret error  */
>> >          goto done;
>> > @@ -2274,48 +2295,46 @@ ad_gpo_populate_candidate_gpos(TALLOC_CTX *mem_ctx,
>> 
>> [...]
>> 
>> > @@ -2709,32 +2734,23 @@ ad_gpo_get_gpo_attrs_done(struct tevent_req
>> > *subreq)
>> >      }
>> >  
>> >      file_sys_path = talloc_strdup(gp_gpo, raw_file_sys_path);
>> > -    ad_gpo_convert_to_smb_uri(state, state->server_hostname,
>> > file_sys_path,
>> > -                              &smb_uri, &unix_path);
>> >  
>> > -    gp_gpo->gpo_file_sys_path = talloc_asprintf(gp_gpo, "%s/Machine",
>> > -                                                smb_uri);
>> > -    if (gp_gpo->gpo_file_sys_path == NULL) {
>> > -        ret = ENOMEM;
>> > -        goto done;
>> > -    }
>> > +    ad_gpo_extract_smb_components(state, state->server_hostname,
>> > +                                  file_sys_path, &smb_server,
>> > +                                  &smb_share, &smb_path);
>> 
>> There is no return code check here.
>
>OK.
>
>> 
>> >  
>> > -    DEBUG(SSSDBG_TRACE_FUNC, "gpo_file_sys_path: %s\n",
>> > -          gp_gpo->gpo_file_sys_path);
>> > +    gp_gpo->smb_server = talloc_strdup(gp_gpo, smb_server);
>> > +    DEBUG(SSSDBG_TRACE_ALL, "smb_server: %s\n", gp_gpo->smb_server);
>> >  
>> > -    gp_gpo->gpo_unix_path = talloc_asprintf(gp_gpo, "%s/Machine",
>> > -                                            unix_path);
>> > -    if (gp_gpo->gpo_unix_path == NULL) {
>> > -        ret = ENOMEM;
>> > -        goto done;
>> > -    }
>> > +    gp_gpo->smb_share = talloc_strdup(gp_gpo, smb_share);
>> > +    DEBUG(SSSDBG_TRACE_ALL, "smb_share: %s\n", gp_gpo->smb_share);
>> >  
>> > -    DEBUG(SSSDBG_TRACE_FUNC, "gpo_unix_path: %s\n",
>> > -          gp_gpo->gpo_unix_path);
>> > +    gp_gpo->smb_path = talloc_strdup(gp_gpo, smb_path);
>> > +    DEBUG(SSSDBG_TRACE_ALL, "smb_path: %s\n", gp_gpo->smb_path);
>> 
>> I wonder if you could call either talloc_steal to move the variables
>> like smb_path, smb_share and smb_server to the gp_gpo context or assign
>> to members of gp_gpo right away when calling
>> ad_gpo_extract_smb_components() instead of allocating the result on
>> state and assigning to local variables. Not a big deal though.
>
>If I store directly to the gp_gpo member variables, they are initially stored
>correctly. However, later on (in ad_gpo_cse_step), the member variables have
>corrupted values. Not sure why. As such, I have left this part of the code
>unchanged.
>
It can be use after free problem or something smilar. It should be visible in
valgrind log. It might help you.

You can add next line to the domain section
command = valgrind -v --leak-check=full --show-reachable=yes --log-file=/var/log/sssd/valgrind_default.log /usr/libexec/sssd/sssd_be --domain default --debug-to-files

You would need to change arguments after sssd_be. pgrep can help you.

sh-4.2# pgrep -af sssd_be
981 /usr/libexec/sssd/sssd_be --domain default --debug-to-files

LS



More information about the sssd-devel mailing list