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

Yassir Elley yelley at redhat.com
Thu Jul 24 00:02:55 UTC 2014



----- Original Message -----
> On Wed, Jul 23, 2014 at 03:03:34AM -0400, 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).
> 
> OK, thank you, I admit I haven't run any tests yet. Can you add a
> comment to that part of code, then? (You don't have to re-send the
> patches now)

Actually, I agree with you that the code is currently confusing, so I have changed it to use a cse_gpo_index initialized to 0. I have also made similar changes to som_index and gpo_index. 

> 
> > > 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.
> 
> I'll try to play with this area of code during my testing. But the
> strdup shouldn't block the inclusion of the patches to master.

Actually, I agree with you here as well, so I have changed the code to avoid the double-allocation (of allocating first to state and then strdup'ing to gp_gpo). 

In a previous email, I mentioned that I tried to directly assign the output values to member variables of gp->gpo, and that this was leading to corruption. Thanks to Lukas's suggestion, I ran valgrind, which showed a "use-after-free" condition leading to corruption, which made my mistake evident, which was that I was still using state (rather than gp_gpo) as the TALLOC_CTX, thereby disrupting the logical memory hierarchy. In other words, I was doing:

    ret = ad_gpo_extract_smb_components(state, state->server_hostname,
                                        file_sys_path, &gp_gpo->smb_server,
                                        &gp_gpo->smb_share, &gp_gpo->smb_path);

instead of the correct solution, which I am now doing, which is:

    ret = ad_gpo_extract_smb_components(gp_gpo, state->server_hostname,
                                        file_sys_path, &gp_gpo->smb_server,
                                        &gp_gpo->smb_share, &gp_gpo->smb_path);


I have attached a revised patch that addresses these two issues. I have also attached the previous (unchanged) sysdb_gpo API patch for your convenience.

Thanks,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-AD-GPO-only-download-policy-files-if-gpo-version-cha.patch
Type: text/x-patch
Size: 47978 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140723/85c6da80/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-GPO-add-sysdb_gpo-support-for-caching-gpo-version.patch
Type: text/x-patch
Size: 10452 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140723/85c6da80/attachment-0001.bin>


More information about the sssd-devel mailing list