Re: libpam segfault when passwd passes NULL pamh (was Re: gcc -O2 produces invalid object code (x86_64, netbsd-5 branch))

[Iain Hibbert <plunky%rya-online.net@localhost>]

On Mon, 8 Mar 2010, Richard Hansen wrote:

> Joerg Sonnenberger wrote:
> > > My interpretation of pam_end(3) is that a NULL pamh is not prohibited,
> > > which would mean that passwd is not doing anything wrong.
> >
> > I disagree. The handle is an abstract opaque type. An implemenation that
> > only ever allows a single PAM context being open could just return NULL all
> > the time for pam_start and it would be within the spec.
>
> Good point.  Let me restate to make sure I understand:  For that hypothetical
> implementation, passing NULL to pam_end() would be appropriate because that's
> what pam_start() returns.  Thus, apps should never test to see if pamh is
> NULL.  Am I correct?

that is correct, pam_err is the success/fail check.

> > The openpam(3) implementation never returns NULL for pam_start and
> > therefore, NULL is not a valid handle to pass down to pam_end.
>
> So callers should only decide whether to call pam_end() or not based on
> pam_start()'s return code, correct?  What do you think about the attached
> patch?
>
> There's also the issue of whether it's OK to pass random garbage to
> pam_strerror().  If it's not OK, then how does one print error messages for
> pam_start() failures?  pam_start() doesn't set pamh on error, so its value
> could be anything.

The pam_check() obfusticates things for no good reason. If you remove that
and do error checking in the clear, you get to not call pam_strerror() in
the case that pamh is not valid and exit straight away, so that you don't
need the extra variable, and you don't need to check pam_err twice either.

I don't know how attached people are to keeping the code synced with
upstream, but perhaps they would accept a rework that made it do the right
thing..

iain