View Issue Details

IDProjectCategoryView StatusLast Update
0000443Pgpool-IIBugpublic2018-11-19 14:04
ReporternagataAssigned Tonagata 
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version 
Target VersionFixed in Version 
Summary0000443: Segmentation fault occurs when a certain Bind message is sent in native replication mode.
DescriptionOne of our clients reported that segmentation fault occurs with a specific query including CURRENT_DATE in native replication mode.

Through my analysis of core file, I found that this occurs in bind_rewrite_timestamp().

#0 0x0000003ab50899d4 in memcpy () at ../sysdeps/x86_64/memcpy.S:444
0000001 0x000000000044588a in bind_rewrite_timestamp (backend=<value optimized out>, message=0xaa0398, orig_msg=<v
alue optimized out>, len=<value optimized out>)
    at /usr/include/bits/string3.h:52
0000002 0x00000000004364f9 in Bind (frontend=0xa9a258, backend=0xa7f350, len=70, contents=0xaa0688 "") at protocol
/pool_proto_modules.c:1343
0000003 0x0000000000436ce7 in ProcessFrontendResponse (frontend=0xa9a258, backend=0xa7f350) at protocol/pool_proto
_modules.c:2396
...

We can reproduce this by a query and pgproto:

 'P' "P1" "DELETE FROM test2 WHERE d = CURRENT_DATE" 0
 'D' 'S' "P1"
 'B' "" "P1" 1 1 0 0
 'E' "" 0
 'C' 'S' ""
 'S'
 'Y'
 'X'

The problem is the Bind ('B') message. The number of parameter format codes is specified to one. This means that the specified format code (this is also one (=binary) in this example) is applied to all parameters. Although the number of the original query's parameter is zero, this message is allowed in the protocol. However, this causes bind_rewrite_timestamp() to call memcpy with a negative value for size_t, because this doesn't suppose the number of parameter format codes is larger than the actual number of the parameter in the original query.

I attached a patch to handle this case properly. Also, some comments and debug codes are added.
Steps To ReproduceTable schema:
 CREATE TABLE test2 (d date);

pgproto:

 'P' "P1" "DELETE FROM test2 WHERE d = CURRENT_DATE" 0
 'D' 'S' "P1"
 'B' "" "P1" 1 1 0 0
 'E' "" 0
 'C' 'S' ""
 'S'
 'Y'
 'X'
TagsNo tags attached.

Activities

nagata

2018-11-05 17:29

developer  

fix_bind_rewrite_timestamp.patch (5,461 bytes)
diff --git a/src/rewrite/pool_timestamp.c b/src/rewrite/pool_timestamp.c
index 98009c19..53a5fa3a 100644
--- a/src/rewrite/pool_timestamp.c
+++ b/src/rewrite/pool_timestamp.c
@@ -326,7 +326,7 @@ rewrite_timestamp_walker(Node *node, void *context)
 					TypeCast	*tc = makeNode(TypeCast);
 					tc->arg = makeTsExpr(ctx);
 					tc->typeName = SystemTypeName("text");
-	
+
 					fcall->funcname = SystemFuncName("timestamptz");
 					fcall->args = list_make1(tc);
 					ctx->rewrite = true;
@@ -850,7 +850,8 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 {
 	int16		 tmp2,
 				 num_params,
-				 num_formats;
+				 num_formats,
+				 num_formats_new;
 	int32		 tmp4;
 	int			 i,
 				 ts_len,
@@ -867,6 +868,7 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 	{
 		fprintf(stderr, "%02x ", orig_msg[i]);
 	}
+	fprintf(stderr, "\n");
 #endif
 
 	ts = get_current_timestamp(backend);
@@ -884,53 +886,78 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 	/* allocate extra memory for parameter formats */
 	num_org_params = message->query_context->num_original_params;
 	new_msg = copy_to = (char *) palloc(*len + sizeof(int16) * (message->num_tsparams + num_org_params));
+
 	copy_from = orig_msg;
 
-	/* portal_name */
+	/* 1. portal_name */
 	copy_len = strlen(copy_from) + 1;
 	memcpy(copy_to, copy_from, copy_len);
-	copy_to += copy_len; copy_from += copy_len;
+	copy_to += copy_len;
+	copy_from += copy_len;
 
-	/* stmt_name */
+	/* 2. stmt_name */
 	copy_len = strlen(copy_from) + 1;
 	memcpy(copy_to, copy_from, copy_len);
-	copy_to += copy_len; copy_from += copy_len;
+	copy_to += copy_len;
+	copy_from += copy_len;
+
+	/* 3. format codes */
 
-	/* format code */
+	/* 3.1. the number of format codes */
 	memcpy(&tmp2, copy_from, sizeof(int16));
-	copy_len = sizeof(int16);
-	tmp2 = num_formats = ntohs(tmp2);
+	num_formats = ntohs(tmp2);
 
-	if (num_formats >= 1)
+	if (num_formats == 0)
 	{
-		/* this means the specified format code is applied all original parameters */
+		/*
+		 * If num_formats is 0, the original message has no parameters or the parameter formats are all text,
+		 * so we don't need additional format codes since timestamp parametes use text as its format.
+		 */
+		num_formats_new = 0;
+	}
+	else
+	{
+		/* If num formats is 1, this means the specified format code is applied for all original parametera,
+		 * so enlarge message length to specify format codes for each of original paramters. */
 		if (num_formats == 1)
-		{
 			*len += (num_org_params - 1) * sizeof(int16);
-			tmp2 += num_org_params - 1;
-		}
 
 		/* enlarge message length for timestamp parameter's formats */
 		*len += message->num_tsparams * sizeof(int16);
-		tmp2 += message->num_tsparams;
+		num_formats_new = num_org_params + message->num_tsparams;
 	}
 
-	tmp2 = htons(tmp2);
-	memcpy(copy_to, &tmp2, copy_len);	/* copy number of format codes */
-	copy_to += copy_len; copy_from += copy_len;
-
-	copy_len = num_formats * sizeof(int16);
-
-	memcpy(copy_to, copy_from, copy_len);		/* copy format codes */
-	copy_to += copy_len; copy_from += copy_len;
+	/* copy the number of format codes */
+	tmp2 = htons(num_formats_new);
+	copy_len = sizeof(int16);
+	memcpy(copy_to, &tmp2, copy_len);
+	copy_to += copy_len;
+	copy_from += copy_len;
 
+	/* 3.2. the format codes */
 	if (num_formats >= 1)
 	{
-		/* copy the specified format code as numbers of original parameters */
+
+		/* If num_formats is 1, copy the specified format code as numbers of original parameters */
 		if (num_formats == 1)
 		{
-			memcpy(copy_to, copy_from, (num_org_params - 1) * sizeof(int16));
-			copy_to += (num_org_params - 1) * sizeof(int16);
+			int16	org_format_code;
+			memcpy(&org_format_code, copy_from, sizeof(int16));
+			copy_from += copy_len;
+
+			for (i=0;i<num_org_params;i++)
+			{
+				memcpy(copy_to, &org_format_code, sizeof(int16));
+				copy_to += sizeof(int16);
+			}
+		}
+		/* otherwize, copy the original format codes as they are*/
+		else
+		{
+			copy_len = num_formats * sizeof(int16);
+			memcpy(copy_to, copy_from, copy_len);
+			copy_to += copy_len;
+			copy_from += copy_len;
 		}
 
 		/* set additional format codes to zero(text) */
@@ -938,6 +965,8 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 		copy_to += sizeof(int16) * message->num_tsparams;
 	}
 
+	/* 4. parameters */
+
 	/* num params */
 	memcpy(&tmp2, copy_from, sizeof(int16));
 	copy_len = sizeof(int16);
@@ -946,7 +975,7 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 	memcpy(copy_to, &tmp2, sizeof(int16));
 	copy_to += copy_len; copy_from += copy_len;
 
-	/* params */
+	/* original params */
 	copy_len = 0;
 	for (i = 0; i < num_params; i++)
 	{
@@ -965,6 +994,7 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 	memcpy(copy_to, copy_from, copy_len);
 	copy_to += copy_len; copy_from += copy_len;
 
+	/* timestamp params */
 	tmp4 = htonl(ts_len);
 	for (i = 0; i < message->num_tsparams; i++)
 	{
@@ -974,12 +1004,21 @@ bind_rewrite_timestamp(POOL_CONNECTION_POOL *backend,
 		copy_to += ts_len;
 	}
 
-	/* result format code */
+	/* 5. result format code */
 	memcpy(&tmp2, copy_from, sizeof(int16));
 	copy_len = sizeof(int16);
 	copy_len += sizeof(int16) * ntohs(tmp2);
 	memcpy(copy_to, copy_from, copy_len);
 
+#ifdef TIMESTAMPDEBUG
+	fprintf(stderr, "message length:%d\n", *len);
+	for(i=0;i<*len;i++)
+	{
+		fprintf(stderr, "%02x ", new_msg[i]);
+	}
+	fprintf(stderr, "\n");
+#endif
+
 	return new_msg;
 }
 

pengbo

2018-11-06 16:53

developer   ~0002239

Thank you.
I will test your patch.

nagata

2018-11-14 11:36

developer   ~0002245

I confirmed that my patch was committed.

https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commitdiff;h=1d8d03237a2ad507f8f1fb8fb0c84b3084f57cdc

Does this means the patch was totally accepted and that this issue was resolved?

If so, I would appreciate it if you could tell me when the fixed version will be released, because our client is suffering from this problem? Even if the minor version up is not planned soon, the new release of RPM, fro example, pgpool-II-pg96-3.6.13-2pgdg.rhel6.x86_64.rpm, would be appreciated.

Thanks,

pengbo

2018-11-14 11:43

developer   ~0002246

Yes, we testet your patch for each branch.
With your patch the issue is resovled. Thank you.

But so far the next minor release or rpm update schedule is not yet decided.
I will let you know as soon as it is decided.

pengbo

2018-11-19 12:14

developer   ~0002270

We decided to do minor release on November 22, 2018.

nagata

2018-11-19 14:04

developer   ~0002272

Thank you for informing me this. I'll announce this to our client.

Thanks!

Issue History

Date Modified Username Field Change
2018-11-05 17:29 nagata New Issue
2018-11-05 17:29 nagata File Added: fix_bind_rewrite_timestamp.patch
2018-11-05 17:31 nagata Description Updated View Revisions
2018-11-06 16:53 pengbo Note Added: 0002239
2018-11-14 11:36 nagata Note Added: 0002245
2018-11-14 11:43 pengbo Note Added: 0002246
2018-11-19 12:14 pengbo Note Added: 0002270
2018-11-19 14:04 nagata Note Added: 0002272
2018-11-19 14:04 nagata Assigned To => nagata
2018-11-19 14:04 nagata Status new => resolved
2018-11-19 14:04 nagata Resolution open => fixed