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.