View Issue Details

IDProjectCategoryView StatusLast Update
0000119Pgpool-IIBugpublic2014-11-28 17:21
Reporterbow.fujitaAssigned Tot-ishii 
PrioritynormalSeverityminorReproducibilitysometimes
Status assignedResolutionopen 
PlatformLinuxOSDebianOS Version7 (wheezy)
Product Version 
Target VersionFixed in Version 
Summary0000119: Signal might be sent to pid=1 (init process) in send_failback_request()
DescriptionI built pgpool-II 3.2.6 from source code and run as streaming-replication mode.
There are two backend PostgreSQL 9.2.4. One is localhost and another is remote.
While I was testing failover and failback features of pgpool-II by restarting each PostgresSQL server repeatedly, Linux suddenly start shutting down.

My environment is not actual Debian distribution but custom (embedded) Linux based on Debian and pgpool is running as root.

I investigated why Linux would be shut down and found that init process (pid=1) received SIGUSR1 upon segfalt occurred in child process of pgpool. Updating pgpool to 3.2.9 wasn't fixed unfortunately.
So I patched some debug codes to pgpool in order to figure out which code sent SIGUSR1 to init process, and I found that it was in send_failback_request() in main.c.

My assumption is, send_failback_request() was called in pool process, however, that pool process had been zombie so ppid would be 1.

Here I attach the patch to prevent kill() from sending a signal to init process.
This patch replaces all kill() with pool_kill() in source tree because I'm not sure which kill() is suspicious.

Regards

TagsNo tags attached.

Activities

bow.fujita

2014-11-05 11:00

reporter  

pool_kill.patch (9,330 bytes)
diff -urN pgpool-II-3.2.9.orig/main.c pgpool-II-3.2.9/main.c
--- pgpool-II-3.2.9.orig/main.c	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/main.c	2014-11-04 17:03:29.506040851 -0800
@@ -368,7 +368,7 @@
 					exit(1);
 				}
 
-				if (kill(pid, SIGHUP) == -1)
+				if (pool_kill(pid, SIGHUP) == -1)
 				{
 					pool_error("could not reload configuration file pid: %d. reason: %s", pid, strerror(errno));
 					pool_shmem_exit(1);
@@ -399,7 +399,7 @@
 		pid = read_pid_file();
 		if (pid > 0)
 		{
-			if (kill(pid, 0) == 0)
+			if (pool_kill(pid, 0) == 0)
 			{
 				fprintf(stderr, "pid file found. is another pgpool(%d) is running?\n", pid);
 				exit(1);
@@ -978,7 +978,7 @@
 		exit(1);
 	}
 
-	if (kill(pid, stop_sig) == -1)
+	if (pool_kill(pid, stop_sig) == -1)
 	{
 		pool_error("could not stop pid: %d. reason: %s", pid, strerror(errno));
 		pool_shmem_exit(1);
@@ -987,7 +987,7 @@
 
 	fprintf(stderr, "stop request sent to pgpool. waiting for termination...");
 
-	while (kill(pid, 0) == 0)
+	while (pool_kill(pid, 0) == 0)
 	{
 		fprintf(stderr, ".");
 		sleep(1);
@@ -1428,7 +1428,7 @@
 			pid_t pid = process_info[i].pid;
 			if (pid)
 			{
-				kill(pid, SIGTERM);
+				pool_kill(pid, SIGTERM);
 			}
 		}
 		while (wait(NULL) > 0)
@@ -1506,7 +1506,7 @@
 	{
 		if (!pool_config->use_watchdog || WD_OK == wd_degenerate_backend_set(node_id_set, count))
 		{
-			kill(parent, SIGUSR1);
+			pool_kill(parent, SIGUSR1);
 		}
 	}
 
@@ -1537,7 +1537,7 @@
 
 	if (!pool_config->use_watchdog || WD_OK == wd_promote_backend(node_id))
 	{
-		kill(parent, SIGUSR1);
+		pool_kill(parent, SIGUSR1);
 	}
 	pool_semaphore_unlock(REQUEST_INFO_SEM);
 }
@@ -1562,7 +1562,7 @@
 	{
 		return;
 	}
-	kill(parent, SIGUSR1);
+	pool_kill(parent, SIGUSR1);
 }
 
 static RETSIGTYPE exit_handler(int sig)
@@ -1607,17 +1607,17 @@
 		pid_t pid = process_info[i].pid;
 		if (pid)
 		{
-			kill(pid, sig);
+			pool_kill(pid, sig);
 		}
 	}
 
-	kill(pcp_pid, sig);
-	kill(worker_pid, sig);
+	pool_kill(pcp_pid, sig);
+	pool_kill(worker_pid, sig);
 
 	if (pool_config->use_watchdog)
 	{
 		pool_log("watchdog_pid: %d", watchdog_pid);
-		kill(watchdog_pid, sig);
+		pool_kill(watchdog_pid, sig);
 	}
 
 	POOL_SETMASK(&UnBlockSig);
@@ -1704,7 +1704,7 @@
 	if (getpid() != mypid)
 	{
 		pool_debug("failover_handler: I am not parent");
-		kill(pcp_pid, SIGUSR2);
+		pool_kill(pcp_pid, SIGUSR2);
 		return;
 	}
 
@@ -1714,7 +1714,7 @@
 	if (exiting)
 	{
 		pool_debug("failover_handler called while exiting");
-		kill(pcp_pid, SIGUSR2);
+		pool_kill(pcp_pid, SIGUSR2);
 		return;
 	}
 
@@ -1724,7 +1724,7 @@
 	if (switching)
 	{
 		pool_debug("failover_handler called while switching");
-		kill(pcp_pid, SIGUSR2);
+		pool_kill(pcp_pid, SIGUSR2);
 		return;
 	}
 
@@ -1734,7 +1734,7 @@
 	{
 		pool_semaphore_unlock(REQUEST_INFO_SEM);
 		kill_all_children(SIGUSR1);
-		kill(pcp_pid, SIGUSR2);
+		pool_kill(pcp_pid, SIGUSR2);
 		return;
 	}
 
@@ -1762,7 +1762,7 @@
 			else
 				pool_error("failover_handler: invalid node_id %d status:%d MAX_NUM_BACKENDS: %d", node_id,
 						   BACKEND_INFO(node_id).backend_status, MAX_NUM_BACKENDS);
-			kill(pcp_pid, SIGUSR2);
+			pool_kill(pcp_pid, SIGUSR2);
 			switching = 0;
 			Req_info->switching = false;
 			return;
@@ -1787,7 +1787,7 @@
 		{
 			pool_log("failover: no backends are promoted");
 			pool_semaphore_unlock(REQUEST_INFO_SEM);
-			kill(pcp_pid, SIGUSR2);
+			pool_kill(pcp_pid, SIGUSR2);
 			switching = 0;
 			Req_info->switching = false;
 			return;
@@ -1818,7 +1818,7 @@
 		{
 			pool_log("failover: no backends are degenerated");
 			pool_semaphore_unlock(REQUEST_INFO_SEM);
-			kill(pcp_pid, SIGUSR2);
+			pool_kill(pcp_pid, SIGUSR2);
 			switching = 0;
 			Req_info->switching = false;
 			return;
@@ -1873,7 +1873,7 @@
 			pool_semaphore_unlock(REQUEST_INFO_SEM);
 			switching = 0;
 			Req_info->switching = false;
-			kill(pcp_pid, SIGUSR2);
+			pool_kill(pcp_pid, SIGUSR2);
 			switching = 0;
 			Req_info->switching = false;
 			return;
@@ -1907,7 +1907,7 @@
 			pid_t pid = process_info[i].pid;
 			if (pid)
 			{
-				kill(pid, SIGQUIT);
+				pool_kill(pid, SIGQUIT);
 				pool_debug("failover_handler: kill %d", pid);
 			}
 		}
@@ -2041,7 +2041,7 @@
 	/*
 	 * Send restart request to worker child.
 	 */
-	kill(worker_pid, SIGUSR1);
+	pool_kill(worker_pid, SIGUSR1);
 
 	if (Req_info->kind == NODE_UP_REQUEST)
 	{
@@ -2068,14 +2068,14 @@
 	/* kick wakeup_handler in pcp_child to notice that
 	 * failover/failback done
 	 */
-	kill(pcp_pid, SIGUSR2);
+	pool_kill(pcp_pid, SIGUSR2);
 
 	sleep(1);
 
 	/*
 	 * Send restart request to pcp child.
 	 */
-	kill(pcp_pid, SIGUSR1);
+	pool_kill(pcp_pid, SIGUSR1);
 	for (;;)
 	{
 		sts = waitpid(pcp_pid, &status, 0);
@@ -2475,7 +2475,7 @@
 	kill_all_children(SIGHUP);
 
 	if (worker_pid)
-		kill(worker_pid, SIGHUP);
+		pool_kill(worker_pid, SIGHUP);
 }
 
 static void kill_all_children(int sig)
@@ -2488,13 +2488,13 @@
 		pid_t pid = process_info[i].pid;
 		if (pid)
 		{
-			kill(pid, sig);
+			pool_kill(pid, sig);
 		}
 	}
 
 	/* make PCP process reload as well */
 	if (sig == SIGHUP)
-		kill(pcp_pid, sig);
+		pool_kill(pcp_pid, sig);
 }
 
 /*
@@ -2561,6 +2561,18 @@
 }
 
 /*
+ * prevents kill() from sending a signal to init process (PID=1).
+ */
+int pool_do_kill(pid_t pid, int sig, const char *file, int line)
+{
+	if (pid == 1) {
+		pool_error("block sending a signal %d to init at %s:%d", sig, file, line);
+		return EINVAL;
+	}
+	return kill(pid, sig);
+}
+
+/*
  * get_config_file_name: return full path of pgpool.conf.
  */
 char *get_config_file_name(void)
diff -urN pgpool-II-3.2.9.orig/pcp_child.c pgpool-II-3.2.9/pcp_child.c
--- pgpool-II-3.2.9.orig/pcp_child.c	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/pcp_child.c	2014-11-04 17:04:41.226044731 -0800
@@ -806,17 +806,17 @@
 				if (mode == 's')
 				{
 					pool_debug("pcp_child: sending SIGTERM to the parent process(%d)", ppid);
-					kill(ppid, SIGTERM);
+					pool_kill(ppid, SIGTERM);
 				}
 				else if (mode == 'f')
 				{
 					pool_debug("pcp_child: sending SIGINT to the parent process(%d)", ppid);
-					kill(ppid, SIGINT);
+					pool_kill(ppid, SIGINT);
 				}
 				else if (mode == 'i')
 				{
 					pool_debug("pcp_child: sending SIGQUIT to the parent process(%d)", ppid);
-					kill(ppid, SIGQUIT);
+					pool_kill(ppid, SIGQUIT);
 				}
 				else
 				{
diff -urN pgpool-II-3.2.9.orig/pool.h pgpool-II-3.2.9/pool.h
--- pgpool-II-3.2.9.orig/pool.h	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/pool.h	2014-11-04 17:04:06.490042866 -0800
@@ -619,6 +619,9 @@
 
 /* main.c */
 extern void pool_sleep(unsigned int second);
+/* DO NOT call pool_do_kill() directly, use pool_kill() macro instead. */
+extern int pool_do_kill(pid_t pid, int sig, const char *file, int line);
+#define pool_kill(pid, sig)	pool_do_kill(pid, sig, __FILE__, __LINE__)
 
 /* pool_worker_child.c */
 extern void do_worker_child(void);
diff -urN pgpool-II-3.2.9.orig/pool_process_query.c pgpool-II-3.2.9/pool_process_query.c
--- pgpool-II-3.2.9.orig/pool_process_query.c	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/pool_process_query.c	2014-11-04 17:05:00.810045783 -0800
@@ -469,7 +469,7 @@
 
 		pool_debug("Query: sending HUP signal to parent");
 
-		kill(getppid(), SIGHUP);        /* send HUP signal to parent */
+		pool_kill(getppid(), SIGHUP);        /* send HUP signal to parent */
 
 		/* we need to loop over here since we will get HUP signal while sleeping */
 		while (stime > 0)
diff -urN pgpool-II-3.2.9.orig/pool_proto_modules.c pgpool-II-3.2.9/pool_proto_modules.c
--- pgpool-II-3.2.9.orig/pool_proto_modules.c	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/pool_proto_modules.c	2014-11-04 17:05:15.714046594 -0800
@@ -432,7 +432,7 @@
 			pool_debug("Query: sending SIGUSR1 signal to parent");
 
 			Req_info->kind = CLOSE_IDLE_REQUEST;
-			kill(getppid(), SIGUSR1);		/* send USR1 signal to parent */
+			pool_kill(getppid(), SIGUSR1);		/* send USR1 signal to parent */
 
 			/* we need to loop over here since we will get USR1 signal while sleeping */
 			while (stime > 0)
diff -urN pgpool-II-3.2.9.orig/recovery.c pgpool-II-3.2.9/recovery.c
--- pgpool-II-3.2.9.orig/recovery.c	2014-09-05 07:21:54.000000000 -0700
+++ pgpool-II-3.2.9/recovery.c	2014-11-04 17:05:20.994046880 -0800
@@ -218,7 +218,7 @@
 	}
 
 	*InRecovery = RECOVERY_INIT;
-	kill(getppid(), SIGUSR2);
+	pool_kill(getppid(), SIGUSR2);
 }
 
 /*
diff -urN pgpool-II-3.2.9.orig/watchdog/watchdog.c pgpool-II-3.2.9/watchdog/watchdog.c
--- pgpool-II-3.2.9.orig/watchdog/watchdog.c	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/watchdog/watchdog.c	2014-11-04 17:05:37.234047739 -0800
@@ -74,7 +74,7 @@
 
 	wd_notice_server_down();
 
-	kill (child_pid, exit_signo);
+	pool_kill(child_pid, exit_signo);
 	child_wait(0);
 
 	exit(0);
diff -urN pgpool-II-3.2.9.orig/watchdog/wd_child.c pgpool-II-3.2.9/watchdog/wd_child.c
--- pgpool-II-3.2.9.orig/watchdog/wd_child.c	2014-09-05 07:26:12.000000000 -0700
+++ pgpool-II-3.2.9/watchdog/wd_child.c	2014-11-04 17:05:47.330048295 -0800
@@ -223,7 +223,7 @@
 		case WD_END_RECOVERY:
 			send_packet.packet_no = WD_NODE_READY;
 			*InRecovery = RECOVERY_INIT;
-			kill(wd_ppid, SIGUSR2);
+			pool_kill(wd_ppid, SIGUSR2);
 			break;
 		case WD_FAILBACK_REQUEST:
 			node = &(recv_pack->wd_body.wd_node_info);	
pool_kill.patch (9,330 bytes)

t-ishii

2014-11-06 08:03

developer   ~0000476

Thanks for the report. Definitely sending any signal to process id 1 is no good.
I have took a look at your patch. I think it would be better to replace getppid() where pgpool-II calls kill. My idea is remembering parent process when pgpool-II child is forked off, the use the process id when it needs to send signal to parent.
This would be simple and less invasive.

bow.fujita

2014-11-06 10:39

reporter   ~0000478

Either way is fine as long as no signal will be sent to pid=1.
But I concern about in case a child process remembers its parent pid, no way to make sure that pid is actually its parent process and it's alive right before sending a signal.

t-ishii

2014-11-06 10:43

developer   ~0000479

I wonder why you run pgpool-II as root in the first place.

bow.fujita

2014-11-07 06:09

reporter   ~0000481

Just lazy configuration. I know I should run pgpool-II as non-root then init process would not accept any signal.

Muhammad Usama

2014-11-13 22:25

developer   ~0000491

I have tried to reproduce the issue but unable to do so.
Apparently the scenario could occur, If somehow the child process becomes orphan and then it tries to issue a signal to its parent (getppid ()). So one solution could be to to check if the saved parent pid in the child is same to the one returned by getppid() before proceeding to send the signal, and if the bids differ that would mean the parent has already died and in that case the child go on to commit the suicide.
There is another solution to guard the child process to become orphan by using prctl() [e.g prctl(PR_SET_PDEATHSIG, SIGHUP)] system call, Asking the kernal to deliver a signal when the parent goes down. And once the child receives that signal it stop processing and exit itself. But the problem with this approach is, prctl() is not posix function and only exists in Linux. So this becomes not an option.

t-ishii

2014-11-28 17:21

developer   ~0000497

> Apparently the scenario could occur, If somehow the child process becomes orphan > and then it tries to issue a signal to its parent (getppid ()). So one solution > could be to to check if the saved parent pid in the child is same to the one returned by getppid() before proceeding to send the signal, and if the bids differ that would mean the parent has already died and in that case the child go on to commit the suicide.

So this is the solution?

Issue History

Date Modified Username Field Change
2014-11-05 11:00 bow.fujita New Issue
2014-11-05 11:00 bow.fujita File Added: pool_kill.patch
2014-11-06 08:00 t-ishii Assigned To => t-ishii
2014-11-06 08:00 t-ishii Status new => assigned
2014-11-06 08:03 t-ishii Note Added: 0000476
2014-11-06 10:39 bow.fujita Note Added: 0000478
2014-11-06 10:43 t-ishii Note Added: 0000479
2014-11-07 06:09 bow.fujita Note Added: 0000481
2014-11-13 22:25 Muhammad Usama Note Added: 0000491
2014-11-28 17:21 t-ishii Note Added: 0000497