From owner-freebsd-hackers@FreeBSD.ORG Mon Dec 9 22:44:29 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B73124C2; Mon, 9 Dec 2013 22:44:29 +0000 (UTC) Received: from mx1.stack.nl (relay04.stack.nl [IPv6:2001:610:1108:5010::107]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 4A740173D; Mon, 9 Dec 2013 22:44:29 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id F3756B8059; Mon, 9 Dec 2013 23:44:27 +0100 (CET) Received: by snail.stack.nl (Postfix, from userid 1677) id D330928497; Mon, 9 Dec 2013 23:44:27 +0100 (CET) Date: Mon, 9 Dec 2013 23:44:27 +0100 From: Jilles Tjoelker To: Ed Schouten Subject: Re: misc/183495: utx.active not being updated correctly Message-ID: <20131209224427.GA4744@stack.nl> References: <20131113233608.GA8289@stack.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20131113233608.GA8289@stack.nl> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@freebsd.org, rich@enterprisesystems.net, bug-followup@FreeBSD.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Dec 2013 22:44:29 -0000 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