On 9 Jan, Dag-Erling Smørgrav wrote:
> Don Lewis <truck...@freebsd.org> writes:
>> Dag-Erling Smørgrav <d...@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);
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
