View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000807Pgpool-IIBugpublic2023-07-15 00:112023-08-17 11:19
ReporterMCanivez Assigned Tot-ishii  
PrioritynormalSeveritycrashReproducibilityalways
Status closedResolutionopen 
PlatformlinuxOSdebianOS Version12
Product Version4.4.3 
Target Version4.4.4Fixed in Version4.4.4 
Summary0000807: Child process crashes when backend sends a protocol v2 errorMessage
DescriptionWhen a frontend using v2 protocol version sends a query which is valid SQL but causes the backend to return an error message, the child processes closes the connection due to "invalid memory alloc request size 1163022927"
Steps To Reproduce- start pgpool connecting to a backend which still supports protocol version 2 (my trial is running 8.4)
- connect to pgpool with a psql client using protocol version 2 (I used a VM running debian woody, with the latest version of the "postgresql-client" package for this version)
- Try to execute "select * from non_existing_table;"
- See error message "invalid memory alloc request size 1163022927" in pgpool logs. client will say that connection was unexpectedly cut.
Additional Information"1163022927" is "ERRO" in ascii.
This is caused due to an assumption in "read_kind_from_backend" that an error message received from the backend is a protocol 3 error. Therefore it tries to read the message length according to the protocol 3 specs (first 4 bytes after the kind byte), however in protocol 2 error message the message comes right after the kind byte. It then tries to allocate an array of size 1163022927 which is incorrect, plus palloc only allows sizes <1GB, which causes the child process to crash.
TagsNo tags attached.

Activities

MCanivez

2023-07-15 00:20

reporter   ~0004417

See proposed patch attached. Tried it with my current setup and it made the issue disappear. I also checked other places where I could find string "== 'E'" but all other places seem to correctly handle different protocol versions when they want to parse the message.
0001-Fix-bux-807.patch (2,162 bytes)    
From 0a47b304fe9a7c0f96640dd61acbe33803f98e4f Mon Sep 17 00:00:00 2001
From: Martin Canivez <mcn@melexis.com>
Date: Fri, 14 Jul 2023 17:17:57 +0200
Subject: [PATCH] Fix bux 807

---
 src/protocol/pool_process_query.c | 33 +++++++++++++++++++------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/src/protocol/pool_process_query.c b/src/protocol/pool_process_query.c
index 172b764c..881ad2e5 100644
--- a/src/protocol/pool_process_query.c
+++ b/src/protocol/pool_process_query.c
@@ -3521,19 +3521,26 @@ read_kind_from_backend(POOL_CONNECTION * frontend, POOL_CONNECTION_POOL * backen
 		{
 			if (kind_list[i] == 'E')
 			{
-				pool_read(CONNECTION(backend, i), &len, sizeof(len));
-				unread_len = sizeof(len);
-				unread_p = palloc(ntohl(len));
-				memcpy(unread_p, &len, sizeof(len));
-				len = ntohl(len);
-				len -= 4;
-				unread_p = repalloc(unread_p, sizeof(len) + len);
-				p = pool_read2(CONNECTION(backend, i), len);
-				memcpy(unread_p + sizeof(len), p, len);
-				unread_len += len;
-				error_stat_count_up(i, extract_error_kind(unread_p + sizeof(len), PROTO_MAJOR_V3));
-				pool_unread(CONNECTION(backend, i), unread_p, unread_len);
-				pfree(unread_p);
+				int major = MAJOR(CONNECTION(backend, i));
+				if (major == PROTO_MAJOR_V3) {
+					pool_read(CONNECTION(backend, i), &len, sizeof(len));
+					unread_len = sizeof(len);
+					unread_p = palloc(ntohl(len));
+					memcpy(unread_p, &len, sizeof(len));
+					len = ntohl(len);
+					len -= 4;
+					unread_p = repalloc(unread_p, sizeof(len) + len);
+					p = pool_read2(CONNECTION(backend, i), len);
+					memcpy(unread_p + sizeof(len), p, len);
+					unread_len += len;
+					error_stat_count_up(i, extract_error_kind(unread_p + sizeof(len), PROTO_MAJOR_V3));
+					pool_unread(CONNECTION(backend, i), unread_p, unread_len);
+					pfree(unread_p);
+				} else if (major == PROTO_MAJOR_V2) {
+					unread_p = pool_read_string(CONNECTION(backend, i), &unread_len, 0);
+					error_stat_count_up(i, extract_error_kind(unread_p, PROTO_MAJOR_V2));
+					pool_unread(CONNECTION(backend, i), unread_p, unread_len);
+				}
 			}
 		}
 	}
-- 
2.39.2 (Apple Git-143)
0001-Fix-bux-807.patch (2,162 bytes)   

t-ishii

2023-07-16 12:08

developer   ~0004418

Thank you for the report. I will look into the patch.
BTW,
>- connect to pgpool with a psql client using protocol version 2 (I used a VM running debian woody, with the latest version of the "postgresql-client" package for this version)
How did you force psql to use version 2 protocol?

MCanivez

2023-07-17 16:28

reporter   ~0004419

Hi,

I didn't :) The last version of postgresql-client package for debian woody is version 7.2.1 (at least in the default repos configured for woody), which still uses protocol version 2 (version 3 was introduced in 7.4). That's the image version I used for my docker container (specifically, "debian/eol:woody") where I am running the client, so I was using protocol 2 by default. I'm not aware of a way to use a non-default version, but to be fair I didn't really look into it.

t-ishii

2023-07-17 19:44

developer   ~0004420

> I didn't :) The last version of postgresql-client package for debian woody is version 7.2.1
Oh, OK.
> I'm not aware of a way to use a non-default version, but to be fair I didn't really look into it.
I had spent some time to figure out there's any secret option to force psql/libpq to use v2 protocol but failed. I believe there's no such a thing.
Anyway, your patch looks good and after applying the patch all regression test of pgpool has passed. So at least the patch does not break v3 protocol cases. I am too lazy to set up old postgresql environment here. Therefore I pushed your patch with minor modification so that it follows our coding style (which is same as PostgreSQL). Thank you so much!

Issue History

Date Modified Username Field Change
2023-07-15 00:11 MCanivez New Issue
2023-07-15 00:20 MCanivez Note Added: 0004417
2023-07-15 00:20 MCanivez File Added: 0001-Fix-bux-807.patch
2023-07-16 12:08 t-ishii Note Added: 0004418
2023-07-16 12:08 t-ishii Assigned To => t-ishii
2023-07-16 12:08 t-ishii Status new => assigned
2023-07-17 16:28 MCanivez Note Added: 0004419
2023-07-17 19:44 t-ishii Note Added: 0004420
2023-08-17 11:19 administrator Status assigned => closed
2023-08-17 11:19 administrator Fixed in Version => 4.4.4
2023-08-17 11:19 administrator Target Version => 4.4.4