Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 20 Aug 2014 10:15:26 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        bugzilla-noreply@freebsd.org
Cc:        freebsd-bugs@freebsd.org
Subject:   Re: [Bug 192837] New: [patch] su(1) does not need to fork; it causes terminal problems
Message-ID:  <20140820095222.G900@besplex.bde.org>
In-Reply-To: <bug-192837-8@https.bugs.freebsd.org/bugzilla/>
References:  <bug-192837-8@https.bugs.freebsd.org/bugzilla/>

next in thread | previous in thread | raw e-mail | index | archive | help
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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140820095222.G900>