NetBSD Problem Report #36079

From mrl@sdf.lonestar.org  Sat Mar 24 12:17:32 2007
Return-Path: <mrl@sdf.lonestar.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 567AC63B853
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 24 Mar 2007 12:17:32 +0000 (UTC)
Message-Id: <20070324114943.46FD1268AE@poultrygeist.com>
Date: Sat, 24 Mar 2007 08:15:39 -0400 (EDT)
From: "M. Levinson" <levinsm@users.sourceforge.net>
Reply-To: "M. Levinson" <levinsm@users.sourceforge.net>
To: gnats-bugs@NetBSD.org
Subject: /bin/sh fails to execute EXIT trap handlers for some subshells
X-Send-Pr-Version: 3.95

>Number:         36079
>Category:       bin
>Synopsis:       /bin/sh fails to execute EXIT trap handlers for some subshells
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Mar 24 12:20:00 +0000 2007
>Closed-Date:    Fri Oct 09 21:21:30 +0000 2009
>Last-Modified:  Fri Oct 09 21:21:30 +0000 2009
>Originator:     M. Levinson
>Release:        NetBSD 4.99.16 from 2007-03-23
>Organization:
>Environment:
     $NetBSD: eval.c,v 1.88 2006/10/16 00:36:19 christos Exp $
Architecture: i386
Machine: i386
>Description:
	If the final command to be run by a subshell is a shell function
	or built-in then /bin/sh executes trap handlers for the EXIT
	pseudo-signal before that subshell exits (as expected). But if the
	final command to be run by the subshell is an external program then
	an identical trap handler won't be executed; in this case the
	subshell doesn't fork before it execs the program, so of course
	after the exec the subshell never gets a chance to call the
	exitshell() function from src/bin/sh/trap.c. With the patch below
	the subshell will fork in this case, so when the subshell exits it
	will call exitshell() and any trap handler will be executed as
	expected.

>How-To-Repeat:
	This command echoes "exiting" as expected:
		sh -c "( trap 'echo exiting' EXIT; /usr/bin/true; : )"

	But this command does not:
		sh -c "( trap 'echo exiting' EXIT; /usr/bin/true )"

>Fix:

--- src/bin/sh/eval.c	2006-10-19 06:49:12.000000000 -0500
+++ src/bin/sh/eval.c	2007-03-23 13:46:16.000000000 -0400
@@ -814,7 +814,7 @@

 	/* Fork off a child process if necessary. */
 	if (cmd->ncmd.backgnd
-	 || (cmdentry.cmdtype == CMDNORMAL && (flags & EV_EXIT) == 0)
+	 || cmdentry.cmdtype == CMDNORMAL
 	 || ((flags & EV_BACKCMD) != 0
 	    && ((cmdentry.cmdtype != CMDBUILTIN && cmdentry.cmdtype != CMDSPLBLTIN)
 		 || cmdentry.u.bltin == dotcmd

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->closed
State-Changed-By: christos@NetBSD.org
State-Changed-When: Sat, 20 Dec 2008 22:16:29 -0500
State-Changed-Why:
fixed, thanks


From: Christos Zoulas <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36079 CVS commit: src/bin/sh
Date: Sun, 21 Dec 2008 03:15:32 +0000 (UTC)

 Module Name:	src
 Committed By:	christos
 Date:		Sun Dec 21 03:15:32 UTC 2008

 Modified Files:
 	src/bin/sh: eval.c

 Log Message:
 PR/36079: M. Levinson: Disable the optimization of not forking for the last
 command in a subshell, otherwise we miss the exit trap.


 To generate a diff of this commit:
 cvs rdiff -r1.94 -r1.95 src/bin/sh/eval.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Christos Zoulas <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36079 CVS commit: src/bin/sh
Date: Mon, 19 Jan 2009 19:47:11 +0000 (UTC)

 Module Name:	src
 Committed By:	christos
 Date:		Mon Jan 19 19:47:11 UTC 2009

 Modified Files:
 	src/bin/sh: eval.c

 Log Message:
 Revert previous commit that fixes PR/36079 (shell misses exit trap), because
 the fix causes $! to point to the wrong process in pipelines, which is worse.


 To generate a diff of this commit:
 cvs rdiff -r1.95 -r1.96 src/bin/sh/eval.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: closed->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 19 Jan 2009 22:32:50 +0000
State-Changed-Why:
Unfixed :(


From: Jilles Tjoelker <jilles@stack.nl>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/36079
Date: Fri, 12 Jun 2009 00:00:51 +0200

 --pWyiEgJYm5f9v55/
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline

 A patch similar to
 http://www.stack.nl/~jilles/unix/sh-forkiftrapped.patch
 should fix this properly, keeping the shell running if there are any
 traps set. I have run with this on FreeBSD for quite a while and have
 not noticed problems, although it hasn't been committed yet.

 It will need to be changed slightly because NetBSD doesn't have the -T
 option (immediate traps).

 -- 
 Jilles Tjoelker

 --pWyiEgJYm5f9v55/
 Content-Type: text/x-diff; charset=us-ascii
 Content-Disposition: attachment; filename="sh-forkiftrapped.patch"

 Don't skip forking for an external command if any traps are active.

 Example:
   sh -c '(trap "echo trapped" EXIT; sleep 3)'
 now correctly prints "trapped".

 With this check, it is no longer necessary to check for -T
 explicitly in that case.

 diff --git a/eval.c b/eval.c
 --- a/eval.c
 +++ b/eval.c
 @@ -730,7 +730,7 @@ evalcommand(union node *cmd, int flags, 
  	/* Fork off a child process if necessary. */
  	if (cmd->ncmd.backgnd
  	 || (cmdentry.cmdtype == CMDNORMAL
 -	    && ((flags & EV_EXIT) == 0 || Tflag))
 +	    && ((flags & EV_EXIT) == 0 || have_traps()))
  	 || ((flags & EV_BACKCMD) != 0
  	    && (cmdentry.cmdtype != CMDBUILTIN
  		 || cmdentry.u.index == CDCMD
 diff --git a/trap.c b/trap.c
 --- a/trap.c
 +++ b/trap.c
 @@ -222,6 +222,21 @@ clear_traps(void)


  /*
 + * Check if we have any traps enabled.
 + */
 +int
 +have_traps(void)
 +{
 +	char *volatile *tp;
 +
 +	for (tp = trap ; tp <= &trap[NSIG - 1] ; tp++) {
 +		if (*tp && **tp)	/* trap not NULL or SIG_IGN */
 +			return 1;
 +	}
 +	return 0;
 +}
 +
 +/*
   * Set the signal handler for the specified signal.  The routine figures
   * out what it should be set to.
   */
 diff --git a/trap.h b/trap.h
 --- a/trap.h
 +++ b/trap.h
 @@ -39,6 +39,7 @@ extern volatile sig_atomic_t gotwinch;

  int trapcmd(int, char **);
  void clear_traps(void);
 +int have_traps(void);
  void setsignal(int);
  void ignoresig(int);
  void onsig(int);

 --pWyiEgJYm5f9v55/--

State-Changed-From-To: open->closed
State-Changed-By: dsl@NetBSD.org
State-Changed-When: Fri, 09 Oct 2009 21:21:30 +0000
State-Changed-Why:
This was fixed (again) very recently, there is a regression test ...


>Unformatted:

Home	PR Database Search