From owner-cvs-src Mon Mar 10 23:59:18 2003 Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 33D1A37B401; Mon, 10 Mar 2003 23:59:12 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 519EC43FBD; Mon, 10 Mar 2003 23:59:10 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id SAA13776; Tue, 11 Mar 2003 18:59:07 +1100 Date: Tue, 11 Mar 2003 18:59:06 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: David Xu Cc: src-committers@FreeBSD.org, "" , "" Subject: Re: cvs commit: src/usr.bin/su su.c In-Reply-To: <200303110010.h2B0ANe3061768@repoman.freebsd.org> Message-ID: <20030311180357.E23929@gamplex.bde.org> References: <200303110010.h2B0ANe3061768@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-cvs-src@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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