From: Alexander Barton <alex@barton.de>
Date: Wed, 14 Jul 2010 08:29:05 +0000 (+0200)
Subject: Remove Proc_Kill(), use timeout to kill child processes
X-Git-Tag: rel-17-rc1~54
X-Git-Url: https://arthur.barton.de/cgi-bin/gitweb.cgi?p=ngircd-alex.git;a=commitdiff_plain;h=refs%2Fheads%2Fgeneric-proc

Remove Proc_Kill(), use timeout to kill child processes

This avoids a race and potentionally killing the wrong process on
systems that use randomized process IDs; now the child itself is
responsible to exit in a timely manner using SIGALRM.
---

diff --git a/src/ngircd/conn.c b/src/ngircd/conn.c
index d8df6274..58a3cbfd 100644
--- a/src/ngircd/conn.c
+++ b/src/ngircd/conn.c
@@ -1091,10 +1091,6 @@ Conn_Close( CONN_ID Idx, const char *LogMsg, const char *FwdMsg, bool InformClie
 		    in_k, out_k);
 	}
 
-	/* Kill possibly running subprocess */
-	if (Proc_InProgress(&My_Connections[Idx].proc_stat))
-		Proc_Kill(&My_Connections[Idx].proc_stat);
-
 	/* Servers: Modify time of next connect attempt? */
 	Conf_UnsetServer( Idx );
 
diff --git a/src/ngircd/irc-login.c b/src/ngircd/irc-login.c
index b1b739b8..03fea99a 100644
--- a/src/ngircd/irc-login.c
+++ b/src/ngircd/irc-login.c
@@ -789,7 +789,10 @@ Hello_User(CLIENT * Client)
 		return DISCONNECTED;
 	}
 
-	pid = Proc_Fork(Conn_GetProcStat(conn), pipefd, cb_Read_Auth_Result);
+	/* Fork child process for PAM authentication; and make sure that the
+	 * process timeout is set higher than the login timeout! */
+	pid = Proc_Fork(Conn_GetProcStat(conn), pipefd,
+			cb_Read_Auth_Result, Conf_PongTimeout + 1);
 	if (pid > 0) {
 		LogDebug("Authenticator for connection %d created (PID %d).",
 			 conn, pid);
diff --git a/src/ngircd/proc.c b/src/ngircd/proc.c
index 1e8cac36..dbcff6f1 100644
--- a/src/ngircd/proc.c
+++ b/src/ngircd/proc.c
@@ -23,6 +23,7 @@
 
 #include "log.h"
 #include "io.h"
+#include "conn.h"
 
 #include "exp.h"
 #include "proc.h"
@@ -42,7 +43,7 @@ Proc_InitStruct (PROC_STAT *proc)
  * Fork a child process.
  */
 GLOBAL pid_t
-Proc_Fork(PROC_STAT *proc, int *pipefds, void (*cbfunc)(int, short))
+Proc_Fork(PROC_STAT *proc, int *pipefds, void (*cbfunc)(int, short), int timeout)
 {
 	pid_t pid;
 
@@ -67,7 +68,10 @@ Proc_Fork(PROC_STAT *proc, int *pipefds, void (*cbfunc)(int, short))
 	case 0:
 		/* New child process: */
 		signal(SIGTERM, Proc_GenericSignalHandler);
+		signal(SIGALRM, Proc_GenericSignalHandler);
 		close(pipefds[0]);
+		alarm(timeout);
+		Conn_CloseAllSockets();
 		return 0;
 	}
 
@@ -87,21 +91,6 @@ Proc_Fork(PROC_STAT *proc, int *pipefds, void (*cbfunc)(int, short))
 	return pid;
 }
 
-/**
- * Kill forked child process.
- */
-GLOBAL void
-Proc_Kill(PROC_STAT *proc)
-{
-	assert(proc != NULL);
-
-	if (proc->pipe_fd > 0)
-		io_close(proc->pipe_fd);
-	if (proc->pid > 0)
-		kill(proc->pid, SIGTERM);
-	Proc_InitStruct(proc);
-}
-
 /**
  * Generic signal handler for forked child processes.
  */
@@ -112,6 +101,11 @@ Proc_GenericSignalHandler(int Signal)
 	case SIGTERM:
 #ifdef DEBUG
 		Log_Subprocess(LOG_DEBUG, "Child got TERM signal, exiting.");
+#endif
+		exit(1);
+	case SIGALRM:
+#ifdef DEBUG
+		Log_Subprocess(LOG_DEBUG, "Child got ALARM signal, exiting.");
 #endif
 		exit(1);
 	}
@@ -119,7 +113,7 @@ Proc_GenericSignalHandler(int Signal)
 
 /**
  * Read bytes from a pipe of a forked child process.
- * In addition, this function makes sure that the child process is dead
+ * In addition, this function makes sure that the child process is ignored
  * after all data has been read or a fatal error occurred.
  */
 GLOBAL size_t
@@ -142,7 +136,7 @@ Proc_Read(PROC_STAT *proc, void *buffer, size_t buflen)
 	else if (bytes_read == 0)
 		LogDebug("Can't read from child process %ld: EOF", proc->pid);
 #endif
-	Proc_Kill(proc);
+	Proc_InitStruct(proc);
 	return (size_t)bytes_read;
 }
 
diff --git a/src/ngircd/proc.h b/src/ngircd/proc.h
index 40a2c292..57612f17 100644
--- a/src/ngircd/proc.h
+++ b/src/ngircd/proc.h
@@ -26,9 +26,7 @@ typedef struct _Proc_Stat {
 GLOBAL void Proc_InitStruct PARAMS((PROC_STAT *proc));
 
 GLOBAL pid_t Proc_Fork PARAMS((PROC_STAT *proc, int *pipefds,
-			       void (*cbfunc)(int, short)));
-
-GLOBAL void Proc_Kill PARAMS((PROC_STAT *proc));
+			       void (*cbfunc)(int, short), int timeout));
 
 GLOBAL void Proc_GenericSignalHandler PARAMS((int Signal));
 
diff --git a/src/ngircd/resolve.c b/src/ngircd/resolve.c
index b88ec11c..9bc3a87a 100644
--- a/src/ngircd/resolve.c
+++ b/src/ngircd/resolve.c
@@ -12,6 +12,8 @@
  */
 
 
+#define RESOLVER_TIMEOUT (Conf_PongTimeout*3)/4
+
 #include "portab.h"
 
 #include "imp.h"
@@ -33,6 +35,7 @@
 
 #include "array.h"
 #include "conn.h"
+#include "conf.h"
 #include "defines.h"
 #include "log.h"
 #include "ng_ipaddr.h"
@@ -63,7 +66,7 @@ Resolve_Addr(PROC_STAT * s, const ng_ipaddr_t *Addr, int identsock,
 
 	assert(s != NULL);
 
-	pid = Proc_Fork(s, pipefd, cbfunc);
+	pid = Proc_Fork(s, pipefd, cbfunc, RESOLVER_TIMEOUT);
 	if (pid > 0) {
 		LogDebug("Resolver for %s created (PID %d).", ng_ipaddr_tostr(Addr), pid);
 		return true;
@@ -89,7 +92,7 @@ Resolve_Name( PROC_STAT *s, const char *Host, void (*cbfunc)(int, short))
 
 	assert(s != NULL);
 
-	pid = Proc_Fork(s, pipefd, cbfunc);
+	pid = Proc_Fork(s, pipefd, cbfunc, RESOLVER_TIMEOUT);
 	if (pid > 0) {
 		/* Main process */
 #ifdef DEBUG