NetBSD Problem Report #36532

From woods@building.weird.com  Sat Jun 23 18:59:03 2007
Return-Path: <woods@building.weird.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 3541763B882
	for <gnats-bugs@gnats.netbsd.org>; Sat, 23 Jun 2007 18:59:03 +0000 (UTC)
Message-Id: <m1I2Ap4-002IwcC@building.weird.com>
Date: Sat, 23 Jun 2007 14:59:02 -0400 (EDT)
From: "Greg A. Woods" <woods@planix.com>
Sender: "Greg A. Woods" <woods@building.weird.com>
Reply-To: "Greg A. Woods" <woods@planix.com>
To: gnats-bugs@NetBSD.org
Subject: fix for a core dump in /bin/sh dotrap()
X-Send-Pr-Version: 3.95

>Number:         36532
>Category:       bin
>Synopsis:       fix for a core dump in /bin/sh dotrap()
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jun 23 19:00:00 +0000 2007
>Closed-Date:    Sun Jul 22 20:54:33 +0000 2018
>Last-Modified:  Sat Aug 25 14:45:01 +0000 2018
>Originator:     Greg A. Woods
>Release:        netbsd-4 2007/06/22
>Organization:
Planix, Inc.; Toronto, Ontario; Canada
>Environment:
System: NetBSD 
>Description:

	there's a bug in dotrap() that can cause a core dump

>How-To-Repeat:

>Fix:

	this has been sitting in my local sources for a very long time
	and I recently discovered it still needed pulling up into my
	local netbsd-4 tree too

Index: bin/sh/trap.c
===================================================================
RCS file: /cvs/master/m-NetBSD/main/src/bin/sh/trap.c,v
retrieving revision 1.33
diff -u -r1.33 trap.c
--- bin/sh/trap.c	15 Jul 2005 17:23:48 -0000	1.33
+++ bin/sh/trap.c	10 Feb 2007 17:17:10 -0000
@@ -413,7 +413,19 @@
 		}
 		gotsig[i - 1] = 0;
 		savestatus=exitstatus;
-		evalstring(trap[i], 0);
+/*
+ * #3  0x00000001200162f8 in dotrap ()
+ *     at /building/work/woods/m-NetBSD-1.6/bin/sh/trap.c:398
+ * 398              evalstring(trap[i], 0);
+ * (gdb) print i
+ * $1 = 1
+ * (gdb) print trap[i]
+ * $2 = 0x0
+ */
+		if (trap[i])
+			evalstring(trap[i], 0);
+		else
+			error("got sig %d, but no trap command set!", i);
 		exitstatus=savestatus;
 	}
 done:

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->feedback
State-Changed-By: christos@netbsd.org
State-Changed-When: Sun, 24 Jun 2007 14:35:12 -0400
State-Changed-Why:
do you have a way to reproduce it?


From: "Greg A. Woods" <woods@planix.com>
To: NetBSD GNATS <gnats-bugs@NetBSD.org>
Cc: <gnats-admin@netbsd.org>, <christos@netbsd.org>
Subject: Re: bin/36532 (fix for a core dump in /bin/sh dotrap())
Date: Sun, 24 Jun 2007 15:13:09 -0400

 --pgp-sign-Multipart_Sun_Jun_24_15:13:09_2007-1
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: quoted-printable

 At Sun, 24 Jun 2007 18:35:13 +0000 (UTC), christos@netbsd.org wrote:
 Subject: Re: bin/36532 (fix for a core dump in /bin/sh dotrap())
 >=20
 > Synopsis: fix for a core dump in /bin/sh dotrap()
 >=20
 > State-Changed-From-To: open->feedback
 > State-Changed-By: christos@netbsd.org
 > State-Changed-When: Sun, 24 Jun 2007 14:35:12 -0400
 > State-Changed-Why:
 > do you have a way to reproduce it?

 No, I don't unfortunately.  I think it may only occur in some narrow
 window.  I captured the one core dump and backtrace by pure luck.

 --=20
 						Greg A. Woods

 H:+1 416 218-0098 W:+1 416 489-5852 x122 VE3TCP RoboHack <woods@robohack.ca>
 Planix, Inc. <woods@planix.com>       Secrets of the Weird <woods@weird.com>

 --pgp-sign-Multipart_Sun_Jun_24_15:13:09_2007-1
 Content-Type: application/pgp-signature
 Content-Transfer-Encoding: 7bit

 -----BEGIN PGP SIGNATURE-----
 Version: PGPfreeware 5.0i for non-commercial use
 MessageID: 5rH0OqFgTT6RSisSFW0pRETR7GnWz/Nz

 iQA/AwUBRn7CRWZ9cbd4v/R/EQLSVQCg2AxCP1NVgbNPxaSsZ62iINZsr+8An1iz
 ji/Z9cNp9pC7i0tNM0LZQq1r
 =tu8E
 -----END PGP SIGNATURE-----

 --pgp-sign-Multipart_Sun_Jun_24_15:13:09_2007-1--

State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Thu, 10 Apr 2008 02:21:33 +0000
State-Changed-Why:
Feedback received.


From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/36532 (fix for a core dump in /bin/sh dotrap())
Date: Mon, 22 Feb 2016 01:06:20 +0700

 I don't have a test case to cause this, but I have a hypothesis about
 how it might happen.

 If sh just happens to be executing a "trap - N" command when
 signal N arrives, then trap[N] will be set to NULL, at the
 same time as gotsig[N] (really N-1, but that's irrelevant)
 will get set (along with pendingsigs).

 When the shell gets to process the pending signal, it assumes that
 if trap[N] is NULL, then the signal state for the process is SIG_DFL,
 (which it does get set to, but possibly, just a little too late) and
 so not eligible for delivery to the shell.)

 Assuming this is the cause, then the patch supplied (minus the error()
 when trap[N] is NULL - just do nothing in that case, ignore the signal,
 it is the best we can do (other than possibly kill(getpid(), N) to
 cause the signal to be redelivered, and take its normal default action.

 Another way, would be to do the setsig() first (when going to SIG_DFL
 or SIG_IGN only - though the SIG_IGN case doesn't cause a problem, there
 the trap is just an empty string, which can be processed, doing nothing,
 without problems) then process pending signals (if any) and only after we
 know that we cannot have signal N delivered, and that there isn't one
 pending, then set trap[N] to NULL.

 There is no (rational) way to fix this just by reordering the setsig()
 and trap[N] = NULL statements, the signal might already be pending,
 it could have arrived from any time after the last check of pendingsigs
 until the setsig() is eventually done.

 kre

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36532 CVS commit: src/bin/sh
Date: Sun, 22 Jul 2018 20:43:58 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Sun Jul 22 20:43:58 UTC 2018

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

 Log Message:
 PR bin/36532 (perhaps)

 This is more or less the same patch as provided in the PR
 (just 11 years later, so changed a bit) by woods@...

 Since there is no known way to actually cause the reported crash,
 we may never know if this change actually fixes anything.   But
 even if it doesn't it certainly cannot hurt.

 There is a potential race which could possibly explain the issue
 (see commentary in the PR) which is not easy to avoid - if that is
 the actual cause, this should provide a defence, if not really a fix.


 To generate a diff of this commit:
 cvs rdiff -u -r1.43 -r1.44 src/bin/sh/trap.c

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

State-Changed-From-To: open->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Sun, 22 Jul 2018 20:54:33 +0000
State-Changed-Why:
The patch in the PR (modified) has been applied - eventually.

Normally the PR would be put in feedback state to await testing,
but there does not seem to be any way to do that here (the
bug has no known trigger) and since the fix applied is more or
less the same as what the PR requested, I am simply closing this
PR instead of going via the feedback route.

I did not include the error() call in case the problem would
have occurred, if it is the race that I think would cause this,
that would be inappropriate.  Instead, we simply force the
result when this particular race is run.


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36532 CVS commit: [netbsd-8] src/bin/sh
Date: Sat, 25 Aug 2018 14:41:22 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Aug 25 14:41:22 UTC 2018

 Modified Files:
 	src/bin/sh [netbsd-8]: trap.c

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #987):

 	bin/sh/trap.c: revision 1.44

 PR bin/36532 (perhaps)

 This is more or less the same patch as provided in the PR
 (just 11 years later, so changed a bit) by woods@...

 Since there is no known way to actually cause the reported crash,
 we may never know if this change actually fixes anything.   But
 even if it doesn't it certainly cannot hurt.

 There is a potential race which could possibly explain the issue
 (see commentary in the PR) which is not easy to avoid - if that is
 the actual cause, this should provide a defence, if not really a fix.


 To generate a diff of this commit:
 cvs rdiff -u -r1.40.2.1 -r1.40.2.2 src/bin/sh/trap.c

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

>Unformatted:

Home	PR Database Search