Re: Second shot at PAM
Hi Igor,
On Sun, Oct 29, 2000 at 12:38:32AM -0400, Igor Khavkine wrote:
> Ok, I took took heed of the comments that I recieved and I updated
> the patch. So here it is, less __GNU__ and more common sense. The patch
> should work for Linux and Hurd so it can be incorporated into the debian
> package right away, whether it should be submitted to the upstream maintainers
> should most probably be at the discretion of the debian maintainer.
It looks better now, but still there are some __GNU__ parts which are
unnecessary, and can be fixed nicer. PAM is using autoconf, right?
So I would suggest to add a check for asprintf, or, maybe better, not to use
asprintf at all but malloc (strlen(pwd->pw_dir) + sizeof(USER_RHOSTS_FILE) + 2)
and sprintf. This should be done unconditionally on all systems, because
there is no need to keep the code for a static buffer when the code to
allocate a buffer dynamically works fine on "legacy" system :) also.
This is exactly what you did with termio->termios conversion, and this part
of the patch looks very clean and nice.
For MAXHOSTNAMELEN, I suggest an autoconf check for <net/if.h>, and include
it only #ifdef HAVE_NET_IF_H. This removes one more __GNU__.
For MAXDNAME I suggest #ifndef MAXDNAME instead checking for __GNU__.
For MAXHOSTNAMELEN, dynamic allocation can be enabled for all systems also
(maybe there needs to be an additional check on systems which define
MAXHOSTNAMELEN if the name is not too long, but often this check is done by
library functions, and those errors are handled corrected automatically in
response to ENAMETOOLONG). Otherwise, if you disagree, enabling the dynamic
code for systems which don't define MAXHOSTNAMELEN would also be a solution.
The use of getline is problematic, as it is a GNU extension. You need to
#define _GNU_SOURCE to get it prototyped. There should be an autoconf check
for this. You can enable _GNU_SOURCE in autoconf like this (early in
configure.in):
CFLAGS="-D_GNU_SOURCE $CFLAGS"
although I am not sure if this is the proper solution... mmh.
The use of __GNU__ is justified, as this is a real GNU shortage. But to give
other short failing systems the same advantage this fix gives to us, it
makes sense to use again a feature based check and not a system based one.
So: ifdef __GNU__ -> ifndef SA_RESETHAND etc.
diff -ru Linux-PAM-0.72.orig/modules/pam_unix/unix_chkpwd.c Linux-PAM-0.72/modules/pam_unix/unix_chkpwd.c
--- Linux-PAM-0.72.orig/modules/pam_unix/unix_chkpwd.c Tue Oct 24 23:23:01 2000
+++ Linux-PAM-0.72/modules/pam_unix/unix_chkpwd.c Wed Oct 25 00:37:59 2000
@@ -51,6 +51,11 @@
static void su_sighandler(int sig)
{
+#ifndef SA_RESETHAND
+ /* emulate the behavior of the SA_RESETHAND flag */
+ if (sig == SIGILL || sig == SIGTRAP || sig == SIGBUS || sig == SIGSEGV)
+ signal(sig, SIG_DFL);
+#endif
if (sig > 0) {
_log_err(LOG_NOTICE, "caught signal %d.", sig);
exit(sig);
@@ -66,7 +71,9 @@
*/
(void) memset((void *) &action, 0, sizeof(action));
action.sa_handler = su_sighandler;
+#ifdef SA_RESETHAND
action.sa_flags = SA_RESETHAND;
+#endif
(void) sigaction(SIGILL, &action, NULL);
(void) sigaction(SIGTRAP, &action, NULL);
(void) sigaction(SIGBUS, &action, NULL);
Thanks,
Marcus
--
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann GNU http://www.gnu.org marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de
Reply to: