Re: PAM patch...

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Not sure if this is 7.2.1 material because it is only an error message
> reporting problem. Comments?

Looks like a bug fix to me. Since it can't possibly break anything
except PAM, I'd rate it as low-risk; hence, reasonable to apply to 7.2.*
too, once we're happy with it.

However, I don't like the patch as given. It seems to perpetuate rather
than replace the fundamentally bogus coding style that led to the
problem in the first place. ISTM that we have a sequence of PAM calls
here that each might fail, and if so we want to report an appropriate
message and return STATUS_ERROR, rather than proceed with the next call.
But as coded, the error handling for call X appears after call X+1!
No wonder it was broken; who could understand this?

I think that the coding pattern shown in lines 746-760 is good:

retval = pam_something(params);

if (retval != PAM_SUCCESS)
{
generate error message;
clean up state as needed;
return STATUS_ERROR;
}

and that the right fix is to make each of the subsequent calls be in
this same pattern, not to try to emulate their nonsensical style.

regards, tom lane