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.