Re: PAM patch...

Bruce Momjian <pgman@candle.pha.pa.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