View Issue Details

IDProjectCategoryView StatusLast Update
0000048Pgpool-IIBugpublic2013-02-01 10:28
ReporterchadsAssigned Tonagata 
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
PlatformFedora release 17OSLinux x86_64OS Version3.6.11-1
Product Version 
Target VersionFixed in Version3.2 
Summary0000048: watchdog crash
Descriptionpthread_detach is being used wrong; causes pgpool to segfault.
Steps To Reproduceconfigure watchdog in pgpool.conf
Additional Informationincluded a patch that resolves the issue
TagsNo tags attached.

Activities

chads

2013-01-16 05:44

reporter  

pthread_detach.patch (1,294 bytes)
diff -uNr ../pgpool-II-3.2.1.orig/watchdog/wd_ping.c ./watchdog/wd_ping.c
--- ../pgpool-II-3.2.1.orig/watchdog/wd_ping.c	2012-09-24 20:18:58.000000000 -0500
+++ ./watchdog/wd_ping.c	2013-01-15 13:53:22.031442664 -0600
@@ -3,7 +3,7 @@
  *
  * Handles watchdog connection, and protocol communication with pgpool-II
  *
- * pgpool: a language independent connection pool server for PostgreSQL 
+ * pgpool: a language independent connection pool server for PostgreSQL
  * written by Tatsuo Ishii
  *
  * Copyright (c) 2003-2012	PgPool Global Development Group
@@ -93,7 +93,7 @@
 		}
 		if (cnt >= MAX_WATCHDOG_NUM)
 		{
-			pool_debug("wd_is_upper_ok: trusted server num is out of range(%d)",cnt);	
+			pool_debug("wd_is_upper_ok: trusted server num is out of range(%d)",cnt);
 			break;
 		}
 	}
@@ -156,7 +156,6 @@
 	{
 		rtn = WD_OK;
 	}
-	pthread_detach(thread);
 
 	return rtn;
 }
@@ -175,6 +174,8 @@
 	char result[256];
 	char ping_path[WD_MAX_PATH_LEN];
 
+	pthread_detach(pthread_self());
+
 	snprintf(ping_path,sizeof(ping_path),"%s/ping",pool_config->ping_path);
 	thread_arg = (WdInfo *)arg;
 	memset(result,0,sizeof(result));
@@ -250,7 +251,7 @@
 	sp = ping_data;
 	for ( i = 0 ; i < 4 ; i ++)
 	{
-		sp = strchr(sp,'/');	
+		sp = strchr(sp,'/');
 		if (sp == NULL)
 		{
 			return -1;
pthread_detach.patch (1,294 bytes)

chads

2013-01-16 22:21

reporter   ~0000217

I found a few more instances of pthread_detach being used wrong. I'm attaching an updated patch.

chads

2013-01-16 22:21

reporter  

pthread_detach.patch-20120116 (5,975 bytes)

nagata

2013-01-17 15:21

developer   ~0000218

I agree to remove unnecessary pthread_detach() that cause segfault.
However, adding pthread_detach(pthread_self()) in the thread function
isn't adequate, because the thread should return a value.

I think, we should use only pthread_join, and remove all pthread_detach.

chads

2013-01-17 23:18

reporter   ~0000219

Continued testing revealed problems with using pthread_self(), especially in regards wd_lifecheck. I agree; continue to use pthread_join(), which is required, and remove the pthread_detach() calls.

chads

2013-01-17 23:19

reporter  

pthread_detach-20130117.patch (5,261 bytes)
diff -uNr ../pgpool-II-3.2.1.orig/watchdog/wd_lifecheck.c ./watchdog/wd_lifecheck.c
--- ../pgpool-II-3.2.1.orig/watchdog/wd_lifecheck.c	2012-09-24 20:18:46.000000000 -0500
+++ ./watchdog/wd_lifecheck.c	2013-01-17 07:26:49.753504571 -0600
@@ -3,7 +3,7 @@
  *
  * Handles watchdog connection, and protocol communication with pgpool-II
  *
- * pgpool: a language independent connection pool server for PostgreSQL 
+ * pgpool: a language independent connection pool server for PostgreSQL
  * written by Tatsuo Ishii
  *
  * Copyright (c) 2003-2012	PgPool Global Development Group
@@ -64,7 +64,7 @@
 			{
 				PQclear(res);
 			}
-			if ((status == PGRES_NONFATAL_ERROR )|| 
+			if ((status == PGRES_NONFATAL_ERROR )||
 				(status == PGRES_FATAL_ERROR ))
 			{
 				rtn = WD_NG;
@@ -146,7 +146,7 @@
 	{
 		int result;
 
-		pool_debug("wd_lifecheck: checking pgpool %d (%s:%d)", i, p->hostname, p->pgpool_port);	
+		pool_debug("wd_lifecheck: checking pgpool %d (%s:%d)", i, p->hostname, p->pgpool_port);
 
 		rc = pthread_join(thread[i], (void **)&result);
 		if ((rc != 0) && (errno == EINTR))
@@ -156,7 +156,7 @@
 		}
 		if (result == WD_OK)
 		{
-			pool_debug("wd_lifecheck: OK, status: %d", p->status);	
+			pool_debug("wd_lifecheck: OK, status: %d", p->status);
 
 			p->life = pool_config->wd_life_point;
 			if ((i == 0) &&
@@ -176,7 +176,7 @@
 		}
 		else
 		{
-			pool_debug("wd_lifecheck: NG, status: %d life:%d", p->status, p->life);	
+			pool_debug("wd_lifecheck: NG, status: %d life:%d", p->status, p->life);
 
 			if (p->life > 0)
 			{
@@ -199,7 +199,6 @@
 				}
 			}
 		}
-		pthread_detach(thread[i]);
 		i++;
 		p++;
 	}
@@ -241,8 +240,8 @@
 
 	snprintf(conninfo,sizeof(conninfo),
 		"host='%s' port='%d' dbname='template1' user='%s' password='%s' connect_timeout='%d'",
-		hostname, 
-		port, 
+		hostname,
+		port,
 		pool_config->recovery_user,
 		pool_config->recovery_password,
 		pool_config->wd_interval / 2 + 1);
diff -uNr ../pgpool-II-3.2.1.orig/watchdog/wd_packet.c ./watchdog/wd_packet.c
--- ../pgpool-II-3.2.1.orig/watchdog/wd_packet.c	2012-09-24 20:18:46.000000000 -0500
+++ ./watchdog/wd_packet.c	2013-01-17 07:27:02.352877823 -0600
@@ -3,7 +3,7 @@
  *
  * Handles watchdog connection, and protocol communication with pgpool-II
  *
- * pgpool: a language independent connection pool server for PostgreSQL 
+ * pgpool: a language independent connection pool server for PostgreSQL
  * written by Tatsuo Ishii
  *
  * Copyright (c) 2003-2012	PgPool Global Development Group
@@ -131,7 +131,7 @@
 	/* set add request packet */
 	packet.packet_no = packet_no;
 	memcpy(&(packet.wd_body.wd_info),WD_List,sizeof(WdInfo));
-	/* send packet to all watchdogs */	
+	/* send packet to all watchdogs */
 	rtn = send_packet_4_nodes(&packet, WD_SEND_TO_MASTER );
 	if (rtn == WD_OK)
 	{
@@ -398,7 +398,7 @@
 		FD_ZERO(&wmask);
 		FD_SET(sock,&wmask);
 		rtn = select(sock+1, (fd_set *)NULL, &wmask, (fd_set *)NULL, &timeout);
-	  
+
 		if (rtn < 0 )
 		{
 			if (errno == EAGAIN || errno == EINTR)
@@ -409,7 +409,7 @@
 		}
 		else if (rtn & FD_ISSET(sock, &wmask))
 		{
-			s = send(sock,send_ptr + send_size,buf_size - send_size ,flag); 
+			s = send(sock,send_ptr + send_size,buf_size - send_size ,flag);
 			if (s < 0)
 			{
 				if (errno == EINTR || errno == EAGAIN)
@@ -450,7 +450,7 @@
 	memset(&buf,0,sizeof(WdPacket));
 	for (;;)
 	{
-		r = recv(sock,read_ptr + read_size ,len - read_size, 0); 
+		r = recv(sock,read_ptr + read_size ,len - read_size, 0);
 		if (r < 0)
 		{
 			if (errno == EINTR || errno == EAGAIN)
@@ -465,7 +465,7 @@
 			read_size += r;
 			if (read_size == len)
 			{
-				
+
 				if ((ntohl(buf.packet_no) >= WD_INVALID) &&
 					(ntohl(buf.packet_no) <= WD_READY ))
 				{
@@ -656,7 +656,6 @@
 				rtn = WD_OK;
 			}
 		}
-		pthread_detach(thread[i]);
 		i++;
 	}
 
@@ -849,7 +848,7 @@
 	memcpy(packet.wd_body.wd_node_info.node_id_set,node_id_set,sizeof(int)*count);
 	packet.wd_body.wd_node_info.node_num = count;
 
-	/* send packet to all watchdogs */	
+	/* send packet to all watchdogs */
 	rtn = send_packet_4_nodes(&packet, WD_SEND_TO_MASTER );
 	if (rtn == WD_OK)
 	{
diff -uNr ../pgpool-II-3.2.1.orig/watchdog/wd_ping.c ./watchdog/wd_ping.c
--- ../pgpool-II-3.2.1.orig/watchdog/wd_ping.c	2012-09-24 20:18:58.000000000 -0500
+++ ./watchdog/wd_ping.c	2013-01-17 07:27:19.861398242 -0600
@@ -3,7 +3,7 @@
  *
  * Handles watchdog connection, and protocol communication with pgpool-II
  *
- * pgpool: a language independent connection pool server for PostgreSQL 
+ * pgpool: a language independent connection pool server for PostgreSQL
  * written by Tatsuo Ishii
  *
  * Copyright (c) 2003-2012	PgPool Global Development Group
@@ -93,7 +93,7 @@
 		}
 		if (cnt >= MAX_WATCHDOG_NUM)
 		{
-			pool_debug("wd_is_upper_ok: trusted server num is out of range(%d)",cnt);	
+			pool_debug("wd_is_upper_ok: trusted server num is out of range(%d)",cnt);
 			break;
 		}
 	}
@@ -111,7 +111,6 @@
 		{
 			rtn = WD_OK;
 		}
-		pthread_detach(thread[i]);
 		i++;
 	}
 	free(buf);
@@ -156,7 +155,6 @@
 	{
 		rtn = WD_OK;
 	}
-	pthread_detach(thread);
 
 	return rtn;
 }
@@ -250,7 +248,7 @@
 	sp = ping_data;
 	for ( i = 0 ; i < 4 ; i ++)
 	{
-		sp = strchr(sp,'/');	
+		sp = strchr(sp,'/');
 		if (sp == NULL)
 		{
 			return -1;

nagata

2013-02-01 10:27

developer   ~0000223

I confirmed that your new patch works well.
We'll adopt this into the next release.

Issue History

Date Modified Username Field Change
2013-01-16 05:44 chads New Issue
2013-01-16 05:44 chads File Added: pthread_detach.patch
2013-01-16 10:08 anzai Assigned To => nagata
2013-01-16 10:08 anzai Status new => assigned
2013-01-16 22:21 chads Note Added: 0000217
2013-01-16 22:21 chads File Added: pthread_detach.patch-20120116
2013-01-17 15:21 nagata Note Added: 0000218
2013-01-17 23:18 chads Note Added: 0000219
2013-01-17 23:19 chads File Added: pthread_detach-20130117.patch
2013-02-01 10:27 nagata Note Added: 0000223
2013-02-01 10:28 nagata Status assigned => resolved
2013-02-01 10:28 nagata Fixed in Version => 3.2
2013-02-01 10:28 nagata Resolution open => fixed