From owner-freebsd-bugs@FreeBSD.ORG Wed Aug 20 00:35:28 2014 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 2181E2A3; Wed, 20 Aug 2014 00:35:28 +0000 (UTC) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id D7A893290; Wed, 20 Aug 2014 00:35:27 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id EF782D628E4; Wed, 20 Aug 2014 10:15:26 +1000 (EST) Date: Wed, 20 Aug 2014 10:15:26 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: bugzilla-noreply@freebsd.org Subject: Re: [Bug 192837] New: [patch] su(1) does not need to fork; it causes terminal problems In-Reply-To: Message-ID: <20140820095222.G900@besplex.bde.org> References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=AOuw8Gd4 c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=DHrTdnQQnncA:10 a=P441AsQnjaMA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=6I5d2MoRAAAA:8 a=pGLkceISAAAA:8 a=Frl44mxmImqAwmP_2UsA:9 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 a=MSl-tDqOz04A:10 Cc: freebsd-bugs@freebsd.org X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Aug 2014 00:35:28 -0000 On Tue, 19 Aug 2014 bugzilla-noreply@freebsd.org wrote: Its treatment of mail (both sending and receiving) is even worse than gnats. > Reporter: ta0kira@gmail.com Its initial bug, for sending, is having a sender of bugzilla-nonreply so that you can't see who originated the mail. The POSIX mailing list is even worse, with the originator's name barely visible near the end. > Problem: > > When calling /usr/bin/su, there are several unconditional calls to tcsetpgrp, > which changes control of the terminal. This causes problems when the su call is > a part of a pipeline and other processes in that pipeline require terminal > access. For example, if I run the following: > > root@host$ su -m nobody -c 'find /' | less > > ...less will get stuck in the background. This is a problem when the call to su > is embedded in a script (e.g., root scripts that need to occasionally do > something as a normal user), and that script is a part of a pipeline, because > one can't simply move the rest of the pipeline into the su command. > > The calls to tcsetpgrp are only necessary because su forks and creates a new > process group for the child. Because the child potentially needs the terminal > for authentication or executing the command, it needs terminal control, which > takes it away from whatever process group su is a part of, e.g., a pipeline. > > Solution: > > I don't see a good reason for the fork+setpgid+waitpid code > (https://svnweb.freebsd.org/base/stable/10/usr.bin/su/su.c?revision=256281&view=markup#l445). > Really, the only thing it accomplishes is having the original suid process hang > around until the command finishes, and it prevents the command from being a > part of the pipeline it's embedded in. (e.g., in "su -m nobody -c 'find /' | > less", "find" and "less" will not be in the same process group.) The fork code > causes problems under these limited circumstances, without any apparent > benefit. I therefore suggest that the fork code be removed, providing expected > behavior to su. (Just for comparison, GNU su doesn't fork, and it exhibits the > expected behavior.) I think there is a reason (something to do with pam), but it isn't good and doesn't apply to me. login has similar problems. IIRC, the only problem with login is wasting resources, but su has the terminal control problems that you noticed. Attempts to fix these 10+ years ago didn't work. IIRC the problems are shell-dependent, and changes in su cannot work for all shells. I use these old patches. I forget what the terminal-handling parts do. There are some printfs that are very rarely executed (I haven't noticed them for about 10+ years), and some ifdefs that try to fix the forking case in another way (the code under these is even more rarely executed since I haven't configured it since testing this 10+ years ago). % Index: su.c % =================================================================== % RCS file: /home/ncvs/src/usr.bin/su/su.c,v % retrieving revision 1.75 % diff -u -2 -r1.75 su.c % --- su.c 15 Jun 2004 20:23:02 -0000 1.75 % +++ su.c 16 Jun 2004 05:36:23 -0000 % @@ -1,2 +1,4 @@ % +#define ONEPROC % + % /* % * Copyright (c) 1988, 1993, 1994 % @@ -135,11 +137,11 @@ % uid_t ruid; % pid_t child_pid, child_pgrp, pid; % - int asme, ch, asthem, fastlogin, prio, i, retcode, % + int asme, ch, asthem, fastlogin, forward_stopsig, prio, i, % + retcode, % statusp, setmaclabel; % u_int setwhat; % char *username, *class, shellbuf[MAXPATHLEN]; % const char *p, *user, *shell, *mytty, **nargv; % - struct sigaction sa, sa_int, sa_quit, sa_pipe; % - int temp, fds[2]; % + struct sigaction sa, sa_int, sa_quit; % % shell = class = cleanenv = NULL; % @@ -180,5 +182,4 @@ % if (user == NULL) % usage(); % - /* NOTREACHED */ % % if (strlen(user) > MAXLOGNAME - 1) % @@ -349,27 +350,41 @@ % sigaction(SIGINT, &sa, &sa_int); % sigaction(SIGQUIT, &sa, &sa_quit); % - sigaction(SIGPIPE, &sa, &sa_pipe); % sa.sa_handler = SIG_DFL; % sigaction(SIGTSTP, &sa, NULL); % statusp = 1; % - if (pipe(fds) == -1) { % - PAM_END(); % - err(1, "pipe"); % - } % +#ifdef ONEPROC % + child_pid = 0; % +#else % child_pid = fork(); % +#endif % switch (child_pid) { % default: % sa.sa_handler = SIG_IGN; % sigaction(SIGTTOU, &sa, NULL); % - close(fds[0]); % - setpgid(child_pid, child_pid); % - tcsetpgrp(STDERR_FILENO, child_pid); % - close(fds[1]); % - sigaction(SIGPIPE, &sa_pipe, NULL); % + forward_stopsig = 0; % while ((pid = waitpid(child_pid, &statusp, WUNTRACED)) != -1) { % if (WIFSTOPPED(statusp)) { % - kill(getpid(), SIGSTOP); % - child_pgrp = getpgid(child_pid); % - tcsetpgrp(1, child_pgrp); % + /* % + * XXX this cannot work if stderr is % + * redirected. % + * /bin/sh seems to use fd 3. % + */ % + child_pgrp = tcgetpgrp(STDERR_FILENO); % + fprintf(stderr, % + "su: pgrp/my_pgid/child_pgid before = %d/%d/%d\n", % + tcgetpgrp(STDERR_FILENO), getpgrp(), % + getpgid(child_pid)); % + if (forward_stopsig && % + WSTOPSIG(statusp) != SIGTTIN && % + WSTOPSIG(statusp) != SIGTTOU) % + kill(getpid(), SIGSTOP); % + fprintf(stderr, % + "su: pgrp/my_pgid/child_pgid after = %d/%d/%d\n", % + tcgetpgrp(STDERR_FILENO), getpgrp(), % + getpgid(child_pid)); % + if (forward_stopsig) % + tcsetpgrp(STDERR_FILENO, child_pgrp); % + else % + tcsetpgrp(STDERR_FILENO, child_pid); % kill(child_pid, SIGCONT); % statusp = 1; % @@ -387,8 +402,20 @@ % err(1, "fork"); % case 0: % - close(fds[1]); % - read(fds[0], &temp, 1); % - close(fds[0]); % - sigaction(SIGPIPE, &sa_pipe, NULL); % +#ifndef ONEPROC % + /* % + * Set a new process group so that job control signals for % + * the child cannot affect the parent no matter how the % + * shell implements job control. % + */ % + child_pid = getpid(); % + setpgid(child_pid, child_pid); % + % + /* % + * Switch back to the parent to make the new process group % + * the foreground one (we cannot do it directly). % + */ % + kill(child_pid, SIGSTOP); % +#endif % + % sigaction(SIGINT, &sa_int, NULL); % sigaction(SIGQUIT, &sa_quit, NULL); % Index: login.c % =================================================================== % RCS file: /home/ncvs/src/usr.bin/login/login.c,v % retrieving revision 1.98 % diff -u -2 -r1.98 login.c % --- login.c 26 Jan 2004 20:04:47 -0000 1.98 % +++ login.c 29 Jan 2004 16:14:26 -0000 % @@ -1,2 +1,4 @@ % +#define ONEPROC % + % /*- % * Copyright (c) 1980, 1987, 1988, 1991, 1993, 1994 % @@ -162,5 +164,5 @@ % struct stat st; % int retries, backoff; % - int ask, ch, cnt, quietlog, rootlogin, rval; % + int ask, ch, cnt, quietlog, rootlogin, rval, status; % uid_t uid, euid; % gid_t egid; % @@ -500,5 +502,9 @@ % * pam_close_session() as root. % */ % +#ifdef ONEPROC % + pid = 0; % +#else % pid = fork(); % +#endif % if (pid < 0) { % err(1, "fork"); % @@ -508,5 +514,4 @@ % * session. % */ % - int status; % setproctitle("-%s [pam]", getprogname()); % waitpid(pid, &status, 0); Bruce