From 8f292aaf88ce31061b49aea2aca8ce561640eb05 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Fri, 21 Jun 2024 15:37:25 +0900 Subject: [PATCH] Fix segfault to not use MAIN_NODE macro. Some functions (close_idle_connection(), new_connection() and pool_create_cp()) used MAIN* and VALID_BACKEND where they are not appropriate. MAIN* and VALID_BACKEND are only useful against current connections to backend, not for pooled connections since in pooled connections which backend is the main node or up and running is necessarily same as the current connections to backend. The misuses of those macros sometimes leads to segfault. This patch introduces new in_use_backend_id() which returns the fist node id in use. This commit replaces some of MAIN* with the return value from in_use_backend_id(). Also inappropriate calls to VALID_BACKEND are replaced with CONNECTION_SLOT macro. Problem reported by Emond Papegaaij Discussion: https://www.pgpool.net/pipermail/pgpool-general/2024-June/009176.html [pgpool-general: 9114] Re: Another segmentation fault Backpatch-through: V4.1 --- src/include/protocol/pool_connection_pool.h | 4 +- src/protocol/child.c | 34 ++++++-------- src/protocol/pool_connection_pool.c | 50 ++++++++++++++++----- 3 files changed, 54 insertions(+), 34 deletions(-) diff --git a/src/include/protocol/pool_connection_pool.h b/src/include/protocol/pool_connection_pool.h index 19b8f72af..7f2f75403 100644 --- a/src/include/protocol/pool_connection_pool.h +++ b/src/include/protocol/pool_connection_pool.h @@ -3,7 +3,7 @@ * pgpool: a language independent connection pool server for PostgreSQL * written by Tatsuo Ishii * - * Copyright (c) 2003-2020 PgPool Global Development Group + * Copyright (c) 2003-2024 PgPool Global Development Group * * Permission to use, copy, modify, and distribute this software and * its documentation for any purpose and without fee is hereby @@ -37,4 +37,6 @@ extern int connect_inet_domain_socket_by_port(char *host, int port, bool retry); extern int connect_unix_domain_socket_by_port(int port, char *socket_dir, bool retry); extern int pool_pool_index(void); extern void close_all_backend_connections(void); +extern int in_use_backend_id(POOL_CONNECTION_POOL *pool); + #endif /* pool_connection_pool_h */ diff --git a/src/protocol/child.c b/src/protocol/child.c index d2ee54b58..3caca3d5d 100644 --- a/src/protocol/child.c +++ b/src/protocol/child.c @@ -1151,6 +1151,7 @@ static RETSIGTYPE close_idle_connection(int sig) POOL_CONNECTION_POOL *p = pool_connection_pool; ConnectionInfo *info; int save_errno = errno; + int main_node_id; /* * DROP DATABSE is ongoing. @@ -1158,44 +1159,35 @@ static RETSIGTYPE close_idle_connection(int sig) if (ignore_sigusr1) return; -#ifdef NOT_USED - ereport(DEBUG1, - (errmsg("close connection request received"))); -#endif - for (j = 0; j < pool_config->max_pool; j++, p++) { - if (!MAIN_CONNECTION(p)) + main_node_id = in_use_backend_id(p); + if (main_node_id < 0) + continue; + + if (!CONNECTION_SLOT(p, main_node_id)) continue; - if (!MAIN_CONNECTION(p)->sp) + if (!CONNECTION_SLOT(p, main_node_id)->sp) continue; - if (MAIN_CONNECTION(p)->sp->user == NULL) + if (CONNECTION_SLOT(p, main_node_id)->sp->user == NULL) continue; - if (MAIN_CONNECTION(p)->closetime > 0) /* idle connection? */ + if (CONNECTION_SLOT(p, main_node_id)->closetime > 0) /* idle connection? */ { -#ifdef NOT_USED - ereport(DEBUG1, - (errmsg("closing idle connection"), - errdetail("user: %s database: %s", MAIN_CONNECTION(p)->sp->user, MAIN_CONNECTION(p)->sp->database))); -#endif + bool freed = false; pool_send_frontend_exits(p); for (i = 0; i < NUM_BACKENDS; i++) { - if (!VALID_BACKEND(i)) + if (!CONNECTION_SLOT(p, i)) continue; - if (i == 0) + if (!freed) { - /* - * only first backend allocated the memory for the start - * up packet - */ pool_free_startup_packet(CONNECTION_SLOT(p, i)->sp); CONNECTION_SLOT(p, i)->sp = NULL; - + freed = true; } pool_close(CONNECTION(p, i)); } diff --git a/src/protocol/pool_connection_pool.c b/src/protocol/pool_connection_pool.c index 0d5b832dd..5385803bc 100644 --- a/src/protocol/pool_connection_pool.c +++ b/src/protocol/pool_connection_pool.c @@ -67,6 +67,8 @@ static POOL_CONNECTION_POOL * new_connection(POOL_CONNECTION_POOL * p); static int check_socket_status(int fd); static bool connect_with_timeout(int fd, struct addrinfo *walk, char *host, int port, bool retry); +#define TMINTMAX 0x7fffffff + /* * initialize connection pools. this should be called once at the startup. */ @@ -247,6 +249,7 @@ pool_create_cp(void) POOL_CONNECTION_POOL *oldestp; POOL_CONNECTION_POOL *ret; ConnectionInfo *info; + int main_node_id; POOL_CONNECTION_POOL *p = pool_connection_pool; @@ -259,7 +262,7 @@ pool_create_cp(void) for (i = 0; i < pool_config->max_pool; i++) { - if (MAIN_CONNECTION(p) == NULL) + if (in_use_backend_id(p) < 0) /* is this connection pool out of use? */ { ret = new_connection(p); if (ret) @@ -277,21 +280,25 @@ pool_create_cp(void) * discard it. */ oldestp = p = pool_connection_pool; - closetime = MAIN_CONNECTION(p)->closetime; + closetime = TMINTMAX; pool_index = 0; for (i = 0; i < pool_config->max_pool; i++) { + main_node_id = in_use_backend_id(p); + if (main_node_id < 0) + elog(ERROR, "no in use backend found"); /* this should not happen */ + ereport(DEBUG1, (errmsg("creating connection pool"), errdetail("user: %s database: %s closetime: %ld", - MAIN_CONNECTION(p)->sp->user, - MAIN_CONNECTION(p)->sp->database, - MAIN_CONNECTION(p)->closetime))); + CONNECTION_SLOT(p, main_node_id)->sp->user, + CONNECTION_SLOT(p, main_node_id)->sp->database, + CONNECTION_SLOT(p, main_node_id)->closetime))); - if (MAIN_CONNECTION(p)->closetime < closetime) + if (CONNECTION_SLOT(p, main_node_id)->closetime < closetime) { - closetime = MAIN_CONNECTION(p)->closetime; + closetime = CONNECTION_SLOT(p, main_node_id)->closetime; oldestp = p; pool_index = i; } @@ -299,18 +306,21 @@ pool_create_cp(void) } p = oldestp; + main_node_id = in_use_backend_id(p); + if (main_node_id < 0) + elog(ERROR, "no in use backend found"); /* this should not happen */ pool_send_frontend_exits(p); ereport(DEBUG1, (errmsg("creating connection pool"), errdetail("discarding old %zd th connection. user: %s database: %s", oldestp - pool_connection_pool, - MAIN_CONNECTION(p)->sp->user, - MAIN_CONNECTION(p)->sp->database))); + CONNECTION_SLOT(p, main_node_id)->sp->user, + CONNECTION_SLOT(p, main_node_id)->sp->database))); for (i = 0; i < NUM_BACKENDS; i++) { - if (!VALID_BACKEND(i)) + if (CONNECTION_SLOT(p, i) == NULL) continue; if (!freed) @@ -391,8 +401,6 @@ pool_backend_timer_handler(int sig) void pool_backend_timer(void) { -#define TMINTMAX 0x7fffffff - POOL_CONNECTION_POOL *p = pool_connection_pool; int i, j; @@ -1066,3 +1074,21 @@ close_all_backend_connections(void) POOL_SETMASK(&oldmask); } + +/* + * Return the first node id in use. + * If no node is in use, return -1. + */ +int +in_use_backend_id(POOL_CONNECTION_POOL *pool) +{ + int i; + + for (i = 0; i < NUM_BACKENDS; i++) + { + if (pool->slots[i]) + return i; + } + + return -1; +} -- 2.39.5