Date: Tue, 11 Mar 2003 18:59:06 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: David Xu <davidxu@FreeBSD.org> Cc: src-committers@FreeBSD.org, "" <cvs-src@FreeBSD.org>, "" <cvs-all@FreeBSD.org> Subject: Re: cvs commit: src/usr.bin/su su.c Message-ID: <20030311180357.E23929@gamplex.bde.org> In-Reply-To: <200303110010.h2B0ANe3061768@repoman.freebsd.org> References: <200303110010.h2B0ANe3061768@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 10 Mar 2003, David Xu wrote:
> davidxu 2003/03/10 16:10:23 PST
>
> FreeBSD src repository
>
> Modified files:
> usr.bin/su su.c
> Log:
> Fix long standing job control bug. SIGTSTP shouldn't be ignored.
> Special instructions tested:
> suspend
> stop $$
Er, this can't be right, since it removes the initialization of sa_tstp
as a side effect, so sa_tstp is stack garbage when it is used set
SIGTSTP for the child. I think it may work because errors for the
second setting of SIGTSTP are ignored (all errors from sigaction are
ignored, of course :-(), and SIGTSTP is soon handled correctly by the
shell, but ignoring SIGTSTP was wrong for the inital su process.
The bug is actually a shortstanding job control bug. SIGTSTP started being
ignored in rev.1.53, in at attempt to work around the not-so-shortstanding
PAM bug of gumming up job control by spawning shells instead of exec'ing
them. su hasn't been PAMmed in RELENG_4, so the bug isn't in any usable
release.
> Revision Changes Path
> 1.64 +0 -1 src/usr.bin/su/su.c
Rev.1.53 makes changes of +3 -1 for SIGTSTP. Backing out all these
changes would remove the use of stack garbage. But perhaps SIGTSTP
should have been set to SIG_DFL instead of SIG_IGN for the initial
su process, so su doesn't depend on its initial state. Then restoring
its initial state for child would be almost as dubious as "restoring"
stack garbage, so the sa_tstp variable is not needed eithe way.
Untested patches relative to relative to an old version:
%%%
Index: su.c
===================================================================
RCS file: /home/ncvs/src/usr.bin/su/su.c,v
retrieving revision 1.62
diff -u -2 -r1.62 su.c
--- su.c 23 Oct 2002 03:19:34 -0000 1.62
+++ su.c 11 Mar 2003 07:55:07 -0000
@@ -131,6 +131,5 @@
char *username, *cleanenv, *class, shellbuf[MAXPATHLEN];
const char *p, *user, *shell, *mytty, **nargv;
-
- struct sigaction sa, sa_int, sa_quit, sa_tstp;
+ struct sigaction sa, sa_int, sa_quit;
shell = class = cleanenv = NULL;
@@ -328,6 +327,6 @@
sigaction(SIGINT, &sa, &sa_int);
sigaction(SIGQUIT, &sa, &sa_quit);
- sigaction(SIGTSTP, &sa, &sa_tstp);
-
+ sa.sa_handler = SIG_DFL;
+ sigaction(SIGTSTP, &sa, NULL);
statusp = 1;
child_pid = fork();
@@ -336,5 +335,13 @@
while ((ret_pid = waitpid(child_pid, &statusp, WUNTRACED)) != -1) {
if (WIFSTOPPED(statusp)) {
+ fprintf(stderr,
+ "su: pgrp/my_pgid/child_pgid before = %d/%d/%d\n",
+ tcgetpgrp(1), getpgid(getpid()),
+ getpgid(child_pid));
kill(getpid(), SIGSTOP);
+ fprintf(stderr,
+ "su: pgrp/my_pgid/child_pgid after = %d/%d/%d\n",
+ tcgetpgrp(1), getpgid(getpid()),
+ getpgid(child_pid));
child_pgrp = getpgid(child_pid);
if (tcgetpgrp(1) == getpgrp()) {
@@ -358,5 +365,4 @@
sigaction(SIGINT, &sa_int, NULL);
sigaction(SIGQUIT, &sa_quit, NULL);
- sigaction(SIGTSTP, &sa_tstp, NULL);
/*
* Set all user context except for: Environmental variables
%%%
The patches also reduce nearby style bugs and add old code for debugging
this problem.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-src" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030311180357.E23929>