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.