[SSSD] [PATCH] AD: conflicting gpo policy settings not being resolved correctly

Jakub Hrozek jhrozek at redhat.com
Thu Oct 2 12:06:59 UTC 2014


On Thu, Oct 02, 2014 at 08:02:00AM -0400, Stephen Gallagher wrote:
> 
> 
> 
> On Thu, 2014-10-02 at 11:45 +0200, Jakub Hrozek wrote:
> > On Wed, Oct 01, 2014 at 10:50:26PM -0400, Stephen Gallagher wrote:
> > > Sorry it took me so long to finish this review. The code is mostly
> > > right, but I found three issues that needed to be addressed before we
> > > could commit it.
> > > 
> > > 1) The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
> > > was resulting in the PAM conversation ultimately returning
> > > PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> > > in the 0001 patch attached. Can be applied separately.
> > 
> > ACK
> > 
> > > 
> > > 2) There was an 'attrs' variable being passed in the
> > > sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> > > causing a crash when accessing LDB.
> > > 
> > > 3) The code could not properly handle when a GPO had an explicitly empty
> > > value (such as one might use to expressly disable a setting from a
> > > lower-priority GPO).
> > > 
> > > Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> > > of the changes I made, see
> > > https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> > > my review there as I went through, so I wouldn't miss any corrections
> > > and as a sort of proof-of-concept)
> > 
> > Thank you, the diff looks good to me.
> > 
> > > 
> > > 
> > > I (believe) I have done a very thorough review of this code, so I'd ask
> > > that someone double-check my updates and then we should be good to go
> > > with committing this. It's a pretty important set of fixes.
> > 
> > I will push the second patch if you agree to squash in two additional fixes
> > in the attached diff. One removes an unused function (I have -Werror set
> > for these types of warnings), the other fixes a potential uninitialized
> > pointer access.
> 
> 
> Thanks, good catches. I squashed your patch into the attached ones.

ACK to both!



More information about the sssd-devel mailing list