Bug 12731

Comment 1 Sheldon Hearn 1999-07-21 10:40:11 UTC

Responsible Changed
From-To: freebsd-bugs->sheldonh

I'll take this one, although private mail indicates that David's already 
onto this one. :-) 

These Irish, I tell you! 

Through the SIGCHLD handler flag_reapchild(), reapchild() is being
triggered when the spawn option's /bin/sh exits during or after we call
hosts_access(). This is bad news, since reapchild() is designed to clean
up sep->se_pids and sep->se_numchild.

The SIGCHLD may arrive any time during or after the hosts_access()
call. I can't see why we're interested in it at that stage, though.

Ciao,
Sheldon.

Index: inetd.c
===================================================================
RCS file: /home/ncvs/src/usr.sbin/inetd/inetd.c,v
retrieving revision 1.61
diff -u -d -r1.61 inetd.c
--- inetd.c	1999/07/15 17:01:43	1.61
+++ inetd.c	1999/07/21 11:47:08
@@ -351,7 +351,7 @@
 	struct servtab *sep;
 	struct passwd *pwd;
 	struct group *grp;
-	struct sigaction sa, sapipe;
+	struct sigaction sa, sachld, sapipe;
 	int tmpint, ch, dofork;
 	pid_t pid;
 	char buf[50];
@@ -462,7 +462,7 @@
 	sa.sa_handler = flag_config;
 	sigaction(SIGHUP, &sa, (struct sigaction *)0);
 	sa.sa_handler = flag_reapchild;
-	sigaction(SIGCHLD, &sa, (struct sigaction *)0);
+	sigaction(SIGCHLD, &sa, &sachld);
 	sa.sa_handler = SIG_IGN;
 	sigaction(SIGPIPE, &sa, &sapipe);
 
@@ -643,6 +643,18 @@
 				fromhost(&req);
 				deny_severity = LIBWRAP_DENY_FACILITY|LIBWRAP_DENY_SEVERITY;
 				allow_severity = LIBWRAP_ALLOW_FACILITY|LIBWRAP_ALLOW_SEVERITY;
+				/*
+				 * Somewhere during or after hosts_access(), a shell from
+				 * hosts_options(5) spawn option will generate a SIGCHLD, which
+				 * must be ignored.
+				 */
+#ifdef SANITY_CHECK
+				if (!dofork)
+					syslog(LOG_ERR,
+					    "can't ignore SIGCHLD in listening process");
+				else
+#endif /* SANITY_CHECK */
+				sigaction(SIGCHLD, &sachld, (struct sigaction *)0);
 				denied = !hosts_access(&req);
 				if (denied) {
 				    syslog(deny_severity,

> The SIGCHLD may arrive any time during or after the hosts_access()
> call. I can't see why we're interested in it at that stage, though.

I wonder if we should reset all the signal handlers after the fork,
afterall - we're no longer interested in HUPs, CHLDs and ALRMs at
that stage.

We should also take care to update maxsock correctly, or we should
make most of the descriptors in inetd close-on-exec - at the moment
we sometimes accidently pass signalpipe[1] to children which could
have some bad effects!

	David.

--- inetd.c	1999/07/15 17:01:43	1.61
+++ inetd.c	1999/07/21 12:20:46
@@ -351,7 +351,7 @@
 	struct servtab *sep;
 	struct passwd *pwd;
 	struct group *grp;
-	struct sigaction sa, sapipe;
+	struct sigaction sa, sapipe, sahup, saalrm, sachld;
 	int tmpint, ch, dofork;
 	pid_t pid;
 	char buf[50];
@@ -457,12 +457,12 @@
 	sigaddset(&sa.sa_mask, SIGCHLD);
 	sigaddset(&sa.sa_mask, SIGHUP);
 	sa.sa_handler = flag_retry;
-	sigaction(SIGALRM, &sa, (struct sigaction *)0);
+	sigaction(SIGALRM, &sa, &saalrm);
 	config();
 	sa.sa_handler = flag_config;
-	sigaction(SIGHUP, &sa, (struct sigaction *)0);
+	sigaction(SIGHUP, &sa, &sahup);
 	sa.sa_handler = flag_reapchild;
-	sigaction(SIGCHLD, &sa, (struct sigaction *)0);
+	sigaction(SIGCHLD, &sa, &sachld);
 	sa.sa_handler = SIG_IGN;
 	sigaction(SIGPIPE, &sa, &sapipe);
 
@@ -484,6 +484,8 @@
 	nsock++;
 	if (signalpipe[0] > maxsock)
 	    maxsock = signalpipe[0];
+	if (signalpipe[1] > maxsock)
+	    maxsock = signalpipe[1];
 
 	for (;;) {
 	    int n, ctrl;
@@ -623,6 +625,11 @@
 				for (tmpint = maxsock; tmpint > 2; tmpint--)
 					if (tmpint != ctrl)
 						(void) close(tmpint);
+				alarm(0);
+				sigaction(SIGALRM, &saalrm);
+				sigaction(SIGCHLD, &sachld);
+				sigaction(SIGHUP, &sahup);
+				/* SIGPIPE reset before exec */
 			    }
 			    /*
 			     * Call tcpmux to find the real service to exec.

On Wed, 21 Jul 1999 13:23:52 +0100, David Malone wrote:

> I wonder if we should reset all the signal handlers after the fork,
> afterall - we're no longer interested in HUPs, CHLDs and ALRMs at
> that stage.

Good.

I was talking crap before when I said that we may receive SIGCHLD any
time after hosts_access(). After chatting to a local guru, I've been
made to see that this is impossible.

So when we unload our SIGCHLD handler, we're actually working around a
bug in hosts_access(3), which should save the existing SIGCHLD handler,
use the default handler (SIG_DFL) and then restore the saved handler.

It's only worth noting because someone needs to chat to Wietse about
this.

> +				sigaction(SIGALRM, &saalrm);
> +				sigaction(SIGCHLD, &sachld);
> +				sigaction(SIGHUP, &sahup);

I don't think that the third argument to sigaction is optional, but
that's trivial. :-)

Ciao,
Sheldon.

> So when we unload our SIGCHLD handler, we're actually working around a
> bug in hosts_access(3), which should save the existing SIGCHLD handler,
> use the default handler (SIG_DFL) and then restore the saved handler.
> 
> It's only worth noting because someone needs to chat to Wietse about
> this.

Indeed - there is already a little signal code in the wrapper code,
so it stands some chance of getting fixed.

> > +				sigaction(SIGALRM, &saalrm);
> > +				sigaction(SIGCHLD, &sachld);
> > +				sigaction(SIGHUP, &sahup);
> 
> I don't think that the third argument to sigaction is optional, but
> that's trivial. :-)

Drat - I ment to say the patch was untested ;-)

I'm going to try to write a test service for inetd which can spot stuff
like leaked file discriptors.

	David.

On Wed, 21 Jul 1999 14:24:13 +0100, David Malone wrote:

> I'm going to try to write a test service for inetd which can spot
> stuff like leaked file discriptors.

I'd prefer to fix the signal handling and leaking descriptors in two
different commits. I've already learned the hard way that fixing
multiple problems with a single commit can cause serious head-aches if
you want to MFC just one of the fixes. :-(

Ciao,
Sheldon.

Sheldon Hearn <sheldonh@uunet.co.za> writes:
> 	Notice in /var/log/messages:
> 	Jul 21 11:11:04 axl inetd[18062]: write: Bad file descriptor

Sounds like something incorrectly closes the signal pipe.

DES
-- 
Dag-Erling Smorgrav - des@flood.ping.uio.no

On 21 Jul 1999 15:38:48 +0200, Dag-Erling Smorgrav wrote:

> Sounds like something incorrectly closes the signal pipe.

Actually, what I need to understand is _why_ we need the signal pipe.
The child doesn't care about the parent's HUPs, ALRMs, CHLDs (!) and PIPEs,
and I'm not sure why the parent would need to know about any of these
received by one of its children.

Certainly, just unloading our SIGCHLD handler appears to resolve the
hosts_options(5) spawn problem. So what's with this signal pipe?

Ciao,
Sheldon.

Sheldon Hearn <sheldonh@uunet.co.za> writes:
> Certainly, just unloading our SIGCHLD handler appears to resolve the
> hosts_options(5) spawn problem. So what's with this signal pipe?

The parent needs it. We can of course close it when we fork, but we
should reset all signal handlers first.

DES
-- 
Dag-Erling Smorgrav - des@flood.ping.uio.no

Hi DES,

Are you happy with the following working version of David's patch? It
only addresses the work-around for the unwanted SIGCHLD from a ``spawn''
within hosts_access(3). I'd prefer to have the signalpipe handling fix
in as a separate commit.

Ciao,
Sheldon.

PS: Yes, we also reset the SIGALRM and SIGHUP handlers, but my feeling
    is that child inetd processes should die when they receive these
    signals. We don't want a child inetd rereading its config and if it
    gets a SIGALRM, it certainly isn't because it requested one.

Index: inetd.c
===================================================================
RCS file: /home/ncvs/src/usr.sbin/inetd/inetd.c,v
retrieving revision 1.62
diff -u -d -r1.62 inetd.c
--- inetd.c	1999/07/21 12:19:24	1.62
+++ inetd.c	1999/07/21 15:28:47
@@ -351,7 +351,7 @@
 	struct servtab *sep;
 	struct passwd *pwd;
 	struct group *grp;
-	struct sigaction sa, sapipe;
+	struct sigaction sa, saalrm, sachld, sahup, sapipe;
 	int tmpint, ch, dofork;
 	pid_t pid;
 	char buf[50];
@@ -457,12 +457,12 @@
 	sigaddset(&sa.sa_mask, SIGCHLD);
 	sigaddset(&sa.sa_mask, SIGHUP);
 	sa.sa_handler = flag_retry;
-	sigaction(SIGALRM, &sa, (struct sigaction *)0);
+	sigaction(SIGALRM, &sa, &saalrm);
 	config();
 	sa.sa_handler = flag_config;
-	sigaction(SIGHUP, &sa, (struct sigaction *)0);
+	sigaction(SIGHUP, &sa, &sahup);
 	sa.sa_handler = flag_reapchild;
-	sigaction(SIGCHLD, &sa, (struct sigaction *)0);
+	sigaction(SIGCHLD, &sa, &sachld);
 	sa.sa_handler = SIG_IGN;
 	sigaction(SIGPIPE, &sa, &sapipe);
 
@@ -624,6 +624,10 @@
 				for (tmpint = maxsock; tmpint > 2; tmpint--)
 					if (tmpint != ctrl)
 						(void) close(tmpint);
+				sigaction(SIGALRM, &saalrm, (struct sigaction *)0);
+				sigaction(SIGCHLD, &sachld, (struct sigaction *)0);
+				sigaction(SIGHUP, &sahup, (struct sigaction *)0);
+				/* SIGPIPE reset before exec */
 			    }
 			    /*
 			     * Call tcpmux to find the real service to exec.

Sheldon Hearn <sheldonh@uunet.co.za> writes:
> Are you happy with the following working version of David's patch?

Hmm, looks OK, though I didn't look too close. From what I can see, it
simply stores the original signal handler state and restores it after
a fork?

DES
-- 
Dag-Erling Smorgrav - des@flood.ping.uio.no

On 21 Jul 1999 17:37:24 +0200, Dag-Erling Smorgrav wrote:

> Hmm, looks OK, though I didn't look too close. From what I can see, it
> simply stores the original signal handler state and restores it after
> a fork?

'Zactly. :-)

Ciao,
Sheldon.

Hi DES,

Would you care to comment on David's speculation regarding the purpose
of nsock?

Thanks,
Sheldon.

------- Forwarded Message
Message-ID:  <199907212259.aa16005@salmon.maths.tcd.ie>
Date: Wed, 21 Jul 1999 22:59:49 +0100
From: David Malone <dwmalone@maths.tcd.ie>
To: Sheldon Hearn <sheldonh@uunet.co.za>
Subject: Re: Tester Wanted: PR 12731 (inetd bug) How-To-Repeat 

[...]
> And can you explain nsock's relationship to maxsock for me? I'm not sure
> why we bump it after FD_SET(signalpipe[0], &allsock). Is this right:
> 
> 	maxsock	=	The highest-numbered descriptor
> 	nsock =		The number of descriptors

maxsock seems to have two functions:

	1) maxsock + 1 is the first arg to select, so it must be
		atleast as big as any fd we want to read from.

	2) the highest numbered file discriptor to try and close
		after a fork.

These used to be the same before the signal pipe stuff got added.

As far as I can tell nsock now does nothing useful. It used to be
used to count the number of active sockets, so when they were all
disabeled the code could do a sigpause (presumeably you can't do
a select on no file discriptors, so they had to do this to stop
inetd chewing CPU time). Since it now always has the sigpipe to
select on, nsock doesn't seem to do anything useful.

The only code that uses it just exits with an error if it ever
hits 0 - which it doesn't 'cos we bump it when we open the pipe.
I'd suggest that we cut it out completly.

> I haven't looked at this very carefully and I'm also a little tired. ;-)

I'll proabaly stay at home and do some mathematics tomorrow, rather
than fiddling with computers ;-) However, I'll try to fix the
leaking discriptor, remove nsock and do some tests. If you want to
commit something you could just update maxsock - I believe that
should fix the problem.

Inetd is begining to look like it has been patched too many times
without a general cleanup to make the code more coherent. It should
really be split into functions like NetBSD's I guess. Would there
be an interest in this for -current?

	David.


------- End of Forwarded Message

Sheldon Hearn <sheldonh@uunet.co.za> writes:
> The only code that uses it just exits with an error if it ever
> hits 0 - which it doesn't 'cos we bump it when we open the pipe.

I know, I wrote some of that code :) See the discussions around the
SIGHUP / zombie bug in May. I left the code in there to make sure I
hadn't made any incorrect assumptions.

> I'd suggest that we cut it out completly.

Agreed.

> Inetd is begining to look like it has been patched too many times
> without a general cleanup to make the code more coherent. It should
> really be split into functions like NetBSD's I guess. Would there
> be an interest in this for -current?

Of course.

DES
-- 
Dag-Erling Smorgrav - des@flood.ping.uio.no

Comment 15 Sheldon Hearn 1999-07-22 17:16:45 UTC

State Changed
From-To: open->closed

Both problems (SIGCHLD handling and descriptor leak) fixed in CURRENT 
(rev 1.67 inetd.c) and MFC'd to STABLE (rev 1.46.2.7  inetd.c). Thanks!