Date: Mon, 9 Dec 2013 22:50:01 GMT From: Jilles Tjoelker <jilles@stack.nl> To: freebsd-bugs@FreeBSD.org Subject: Re: misc/183495: utx.active not being updated correctly Message-ID: <201312092250.rB9Mo1m1029191@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR misc/183495; it has been noted by GNATS.
From: Jilles Tjoelker <jilles@stack.nl>
To: Ed Schouten <ed@80386.nl>
Cc: freebsd-hackers@freebsd.org, rich@enterprisesystems.net,
bug-followup@FreeBSD.org
Subject: Re: misc/183495: utx.active not being updated correctly
Date: Mon, 9 Dec 2013 23:44:27 +0100
On Thu, Nov 14, 2013 at 12:36:09AM +0100, Jilles Tjoelker wrote:
> At least on stable/9, the login [pam] process has all UIDs equal to 0
> and cannot be killed by a normal user. However, it can receive SIGHUP if
> the connection is closed. This should not lead to immediate termination,
> so the SIGHUP part of the patch seems correct, although I have not
> tested it.
> I am less sure about the SIGTERM part. Killing a login [pam] process may
> be a "doctor it hurts if I do this" thing. If SIGTERM is not left at its
> default action, perhaps it should kill process(es) associated with the
> session somehow (most simply, by cleaning up PAM and audit and exiting,
> which shuts down the session).
> The only way to avoid problems with SIGKILL would be to put the PAM and
> audit cleanup in a constantly running daemon such as init. I don't think
> this is worth it. Root should not kill -9 login [pam] processes.
After testing by Richard Naill and some more deliberation, I think the
proper direction is to exit login(1) cleanly when SIGHUP or SIGTERM are
received (shutting down utmpx and audit properly). This keeps the
situation for "hung" processes after the connection is closed the same
way as it was with the default action for SIGHUP and SIGTERM.
The below patch (lightly tested) does this. I also replaced all use of
the obsolete signal() function with sigaction() (not only the part where
it is actually required: SIGHUP and SIGTERM must mask the other as well
when caught).
Index: usr.bin/login/login.c
===================================================================
--- usr.bin/login/login.c (revision 259027)
+++ usr.bin/login/login.c (working copy)
@@ -94,6 +94,7 @@ static void refused(const char *, const char *,
static const char *stypeof(char *);
static void sigint(int);
static void timedout(int);
+static void bail_sig(int);
static void usage(void);
#define TTYGRPNAME "tty" /* group to own ttys */
@@ -172,13 +173,18 @@ main(int argc, char *argv[])
login_cap_t *lc = NULL;
login_cap_t *lc_user = NULL;
pid_t pid;
+ sigset_t mask, omask;
+ struct sigaction sa;
#ifdef USE_BSM_AUDIT
char auditsuccess = 1;
#endif
- (void)signal(SIGQUIT, SIG_IGN);
- (void)signal(SIGINT, SIG_IGN);
- (void)signal(SIGHUP, SIG_IGN);
+ sa.sa_flags = SA_RESTART;
+ (void)sigfillset(&sa.sa_mask);
+ sa.sa_handler = SIG_IGN;
+ (void)sigaction(SIGQUIT, &sa, NULL);
+ (void)sigaction(SIGINT, &sa, NULL);
+ (void)sigaction(SIGHUP, &sa, NULL);
if (setjmp(timeout_buf)) {
if (failures)
badlogin(username);
@@ -186,7 +192,8 @@ main(int argc, char *argv[])
timeout);
bail(NO_SLEEP_EXIT, 0);
}
- (void)signal(SIGALRM, timedout);
+ sa.sa_handler = timedout;
+ (void)sigaction(SIGALRM, &sa, NULL);
(void)alarm(timeout);
(void)setpriority(PRIO_PROCESS, 0, 0);
@@ -370,8 +377,15 @@ main(int argc, char *argv[])
/* committed to login -- turn off timeout */
(void)alarm((u_int)0);
- (void)signal(SIGHUP, SIG_DFL);
+ (void)sigemptyset(&mask);
+ (void)sigaddset(&mask, SIGHUP);
+ (void)sigaddset(&mask, SIGTERM);
+ (void)sigprocmask(SIG_BLOCK, &mask, &omask);
+ sa.sa_handler = bail_sig;
+ (void)sigaction(SIGHUP, &sa, NULL);
+ (void)sigaction(SIGTERM, &sa, NULL);
+
endpwent();
#ifdef USE_BSM_AUDIT
@@ -550,10 +564,17 @@ main(int argc, char *argv[])
/*
* Parent: wait for child to finish, then clean up
* session.
+ *
+ * If we get SIGHUP or SIGTERM, clean up the session
+ * and exit right away. This will make the terminal
+ * inaccessible and send SIGHUP to the foreground
+ * process group.
*/
int status;
setproctitle("-%s [pam]", getprogname());
+ (void)sigprocmask(SIG_SETMASK, &omask, NULL);
waitpid(pid, &status, 0);
+ (void)sigprocmask(SIG_BLOCK, &mask, NULL);
bail(NO_SLEEP_EXIT, 0);
}
@@ -627,10 +648,15 @@ main(int argc, char *argv[])
login_close(lc_user);
login_close(lc);
- (void)signal(SIGALRM, SIG_DFL);
- (void)signal(SIGQUIT, SIG_DFL);
- (void)signal(SIGINT, SIG_DFL);
- (void)signal(SIGTSTP, SIG_IGN);
+ sa.sa_handler = SIG_DFL;
+ (void)sigaction(SIGALRM, &sa, NULL);
+ (void)sigaction(SIGQUIT, &sa, NULL);
+ (void)sigaction(SIGINT, &sa, NULL);
+ (void)sigaction(SIGTERM, &sa, NULL);
+ (void)sigaction(SIGHUP, &sa, NULL);
+ sa.sa_handler = SIG_IGN;
+ (void)sigaction(SIGTSTP, &sa, NULL);
+ (void)sigprocmask(SIG_SETMASK, &omask, NULL);
/*
* Login shells have a leading '-' in front of argv[0]
@@ -847,7 +873,7 @@ sigint(int signo __unused)
static int
motd(const char *motdfile)
{
- sig_t oldint;
+ struct sigaction newint, oldint;
FILE *f;
int ch;
@@ -854,10 +880,13 @@ motd(const char *motdfile)
if ((f = fopen(motdfile, "r")) == NULL)
return (-1);
motdinterrupt = 0;
- oldint = signal(SIGINT, sigint);
+ newint.sa_handler = sigint;
+ newint.sa_flags = 0;
+ sigfillset(&newint.sa_mask);
+ sigaction(SIGINT, &newint, &oldint);
while ((ch = fgetc(f)) != EOF && !motdinterrupt)
putchar(ch);
- signal(SIGINT, oldint);
+ sigaction(SIGINT, &oldint, NULL);
if (ch != EOF || ferror(f)) {
fclose(f);
return (-1);
@@ -972,6 +1001,8 @@ pam_cleanup(void)
void
bail(int sec, int eval)
{
+ struct sigaction sa;
+ int signo;
pam_cleanup();
#ifdef USE_BSM_AUDIT
@@ -979,5 +1010,28 @@ bail(int sec, int eval)
audit_logout();
#endif
(void)sleep(sec);
- exit(eval);
+ if (eval < 256)
+ exit(eval);
+ else {
+ signo = eval - 256;
+ sa.sa_handler = SIG_DFL;
+ sa.sa_flags = 0;
+ (void)sigemptyset(&sa.sa_mask);
+ (void)sigaction(signo, &sa, NULL);
+ (void)sigaddset(&sa.sa_mask, signo);
+ (void)sigprocmask(SIG_UNBLOCK, &sa.sa_mask, NULL);
+ raise(signo);
+ exit(128 + signo);
+ }
}
+
+/*
+ * Exit because of a signal.
+ * This is not async-signal safe, so only call async-signal safe functions
+ * while the signal is unmasked.
+ */
+static void
+bail_sig(int signo)
+{
+ bail(NO_SLEEP_EXIT, 256 + signo);
+}
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201312092250.rB9Mo1m1029191>