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(a)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