tech-toolchain archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

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.

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));
-       pam_end(pamh, pam_err);
+       if (pamh) {
+               pam_end(pamh, pam_err);
+               /* pamh is now invalid */
+               pamh = NULL;
+       }
        if (pam_err == PAM_SUCCESS)
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.
+.Fa pamh
+argument must not be NULL.
 .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.
+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.
 function returns one of the following values:
 .Bl -tag -width 18n
+Successful completion.
 Memory buffer error.
Index: dist/openpam/include/security/pam_appl.h
RCS file: /NETBSD-CVS/src/dist/openpam/include/security/pam_appl.h,v
retrieving revision
diff -u -r1.1.1.2 pam_appl.h
--- dist/openpam/include/security/pam_appl.h    27 Jan 2008 00:54:59 -0000
+++ dist/openpam/include/security/pam_appl.h    8 Mar 2010 18:02:13 -0000
@@ -72,8 +72,7 @@
 pam_end(pam_handle_t *_pamh,
-       int _status)
-       OPENPAM_NONNULL((1));
+       int _status);
 pam_get_data(const pam_handle_t *_pamh,

Home | Main Index | Thread Index | Old Index