Re: couldn't log on to my -CURRENT machine after upgrade to latest PAM

On  9 Jan, Dag-Erling Smørgrav wrote:
> Don Lewis <truckman_at_FreeBSD.org> writes:
>> Dag-Erling Smørgrav <des_at_des.no> writes:
>> > The culprit was this commit:
>> > 
>> > http://trac.des.no/openpam/changeset/487/trunk/lib/openpam_configure.c
>> > 
>> > However, I'm not confident that simply reverting this commit is the
>> > right way to go.
>> Thanks for the detective work.  It looks to me like the bug is caused by
>> the change in the openpam_parse_chain() return value.  In the previous
>> code it returned the value of count, which I would guess was greater
>> than zero if it found something.  In that case, the for loop in
>> openpam_load_chain() would be terminated because r != 0.  In the new
>> code, openpam_parse_chain() will return PAM_SUCCESS if it found
>> something, and the loop in openpam_load_chain() will go through another
>> iteration because ret == PAM_SUCCESS.
> 
> Thank you, Captain Obvious.  I am still not confident that simply
> reverting this commit is the right way to go, because it discards
> valuable information when an error occurs, especially if an error occurs
> while parsing an include.

It wasn't so obvious to me, especially with the gratuitous variable
renaming in the diff.

After staring at the code a lot more, I see your point about the loss of
information.  The problem is that openpam_parse_chain() returns
PAM_SUCCESS whether or not if found anything, but we want the loop to
terminate when either an error is detected or if openpam_parse_chain()
actually found something.  Maybe changing the loop exit to something
like this would work:

		if (ret != PAM_SUCCESS || pamh->chains[facility] != NULL)
                	return (ret);