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

[Richard Hansen <rhansen%bbn.com@localhost>]

Joerg Sonnenberger wrote:
> On Mon, Mar 08, 2010 at 10:33:52AM -0500, Richard Hansen wrote:
> > So the bug isn't in gcc, it's in openpam:  either the pam_end(3) man 
> > page should be updated to say that pamh must be non-NULL, or the 
> > attribute should be removed from the declaration.  Which do people 
> > prefer?
>
> I still don't get what makes you point your finger at random stuff and 
> complain that it is broken. What did you do to make pam_start return 
> success and NULL in *pamh?

With the segfault I'm seeing, pam_start() isn't returning success.  The 
problem is that passwd calls pam_end() as part of its cleanup routine, 
and this routine is executed even when pam_start() returns failure.  See 
pwpam_process() in src/usr.bin/passwd/pam_passwd.c.

My interpretation of pam_end(3) is that a NULL pamh is not prohibited, 
which would mean that passwd is not doing anything wrong.

Here is my preference for addressing this issue:
  * the pam_end(3) man page should be updated to say that callers must 
not pass a NULL pamh
  * passwd should be updated to not call pam_end() if pamh is NULL
  * the gcc function attribute on pam_end() should cause gcc to issue 
compiler warnings if someone tries to pass NULL, but it should not cause 
the optimizer to assume pamh is non-NULL

Unfortunately, I don't think it's possible to configure gcc in the above 
way, so there is a choice to make:
  * keep the attribute:  gcc will warn if a caller tries to pass NULL, 
but -O2 will optimize away the test for NULL at the start of pam_end()
  * remove the attribute:  pam_end() will test for NULL even if 
optimization is enabled, but gcc will not issue warnings if callers pass 
NULL

I don't think callers are likely to explicitly pass NULL, so I think it 
would be safer to remove the attribute.

Attached is a patch that does the above.

-Richard

Index: usr.bin/passwd/pam_passwd.c
===================================================================
RCS file: /NETBSD-CVS/src/usr.bin/passwd/pam_passwd.c,v
retrieving revision 1.4
diff -u -r1.4 pam_passwd.c
--- usr.bin/passwd/pam_passwd.c 6 May 2007 09:19:44 -0000       1.4
+++ usr.bin/passwd/pam_passwd.c 8 Mar 2010 18:02:13 -0000
@@ -121,7 +121,11 @@
                    pam_strerror(pamh, pam_err));
 
  end:
-       pam_end(pamh, pam_err);
+       if (pamh) {
+               pam_end(pamh, pam_err);
+               /* pamh is now invalid */
+               pamh = NULL;
+       }
        if (pam_err == PAM_SUCCESS)
                return;
        exit(1);
Index: dist/openpam/doc/man/pam_end.3
===================================================================
RCS file: /NETBSD-CVS/src/dist/openpam/doc/man/pam_end.3,v
retrieving revision 1.5
diff -u -r1.5 pam_end.3
--- dist/openpam/doc/man/pam_end.3      27 Jan 2008 01:22:56 -0000      1.5
+++ dist/openpam/doc/man/pam_end.3      8 Mar 2010 18:02:13 -0000
@@ -56,6 +56,10 @@
 corresponding PAM context, releasing all resources allocated to it.
 .Pp
 The
+.Fa pamh
+argument must not be NULL.
+.Pp
+The
 .Fa status
 argument should be set to the error code returned by the
 last API call before the call to
Index: dist/openpam/doc/man/pam_start.3
===================================================================
RCS file: /NETBSD-CVS/src/dist/openpam/doc/man/pam_start.3,v
retrieving revision 1.6
diff -u -r1.6 pam_start.3
--- dist/openpam/doc/man/pam_start.3    27 Jan 2008 01:22:58 -0000      1.6
+++ dist/openpam/doc/man/pam_start.3    8 Mar 2010 18:02:13 -0000
@@ -77,11 +77,18 @@
 conversation function to use; see
 .Fa pam_conv
 for details.
+.Pp
+On success, the value at
+.Fa pamh
+is set to the newly initialized PAM context.  On error, the value at
+this location is left unchanged.
 .Sh RETURN VALUES
 The
 .Nm
 function returns one of the following values:
 .Bl -tag -width 18n
+.It Bq Er PAM_SUCCESS
+Successful completion.
 .It Bq Er PAM_BUF_ERR
 Memory buffer error.
 .It Bq Er PAM_SYSTEM_ERR
Index: dist/openpam/include/security/pam_appl.h
===================================================================
RCS file: /NETBSD-CVS/src/dist/openpam/include/security/pam_appl.h,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 pam_appl.h
--- dist/openpam/include/security/pam_appl.h    27 Jan 2008 00:54:59 -0000      
1.1.1.2
+++ dist/openpam/include/security/pam_appl.h    8 Mar 2010 18:02:13 -0000
@@ -72,8 +72,7 @@
 
 int
 pam_end(pam_handle_t *_pamh,
-       int _status)
-       OPENPAM_NONNULL((1));
+       int _status);
 
 int
 pam_get_data(const pam_handle_t *_pamh,