From df38b507278cf2f2dffb564b370b6f07c3116079 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Wed, 25 Jan 2023 10:22:41 +0100 Subject: [PATCH v9 2/3] Refactor libpq to store addrinfo in a libpq owned array This refactors libpq to copy addrinfos returned by getaddrinfo to memory owned by us. This refactoring is useful for two upcoming patches, which need to change the addrinfo list in some way. Doing that with the original addrinfo list is risky since we don't control how memory is freed. Also changing the contents of a C array is quite a bit easier than changing a linked list. As a nice side effect of this refactor the is that mechanism for iteration over addresses in PQconnectPoll is now identical to its iteration over hosts. --- src/include/libpq/pqcomm.h | 6 ++ src/interfaces/libpq/fe-connect.c | 107 +++++++++++++++++++++--------- src/interfaces/libpq/libpq-int.h | 6 +- src/tools/pgindent/typedefs.list | 1 + 4 files changed, 87 insertions(+), 33 deletions(-) diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index 66ba359390f..ee28e223bd7 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -27,6 +27,12 @@ typedef struct socklen_t salen; } SockAddr; +typedef struct +{ + int family; + SockAddr addr; +} AddrInfo; + /* Configure the UNIX socket location for the well known port. */ #define UNIXSOCK_PATH(path, port, sockdir) \ diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 97e47f05852..41deeee9a63 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -379,6 +379,7 @@ static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions); static void freePGconn(PGconn *conn); static void closePGconn(PGconn *conn); static void release_conn_addrinfo(PGconn *conn); +static bool store_conn_addrinfo(PGconn *conn, struct addrinfo *addrlist); static void sendTerminateConn(PGconn *conn); static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage); static PQconninfoOption *parse_connection_string(const char *connstr, @@ -2077,7 +2078,7 @@ connectDBComplete(PGconn *conn) time_t finish_time = ((time_t) -1); int timeout = 0; int last_whichhost = -2; /* certainly different from whichhost */ - struct addrinfo *last_addr_cur = NULL; + int last_whichaddr = -2; /* certainly different from whichaddr */ if (conn == NULL || conn->status == CONNECTION_BAD) return 0; @@ -2121,11 +2122,11 @@ connectDBComplete(PGconn *conn) if (flag != PGRES_POLLING_OK && timeout > 0 && (conn->whichhost != last_whichhost || - conn->addr_cur != last_addr_cur)) + conn->whichaddr != last_whichaddr)) { finish_time = time(NULL) + timeout; last_whichhost = conn->whichhost; - last_addr_cur = conn->addr_cur; + last_whichaddr = conn->whichaddr; } /* @@ -2272,9 +2273,9 @@ keep_going: /* We will come back to here until there is /* Time to advance to next address, or next host if no more addresses? */ if (conn->try_next_addr) { - if (conn->addr_cur && conn->addr_cur->ai_next) + if (conn->whichaddr < conn->naddr) { - conn->addr_cur = conn->addr_cur->ai_next; + conn->whichaddr++; reset_connection_state_machine = true; } else @@ -2287,6 +2288,7 @@ keep_going: /* We will come back to here until there is { pg_conn_host *ch; struct addrinfo hint; + struct addrinfo *addrlist; int thisport; int ret; char portstr[MAXPGPATH]; @@ -2327,7 +2329,7 @@ keep_going: /* We will come back to here until there is /* Initialize hint structure */ MemSet(&hint, 0, sizeof(hint)); hint.ai_socktype = SOCK_STREAM; - conn->addrlist_family = hint.ai_family = AF_UNSPEC; + hint.ai_family = AF_UNSPEC; /* Figure out the port number we're going to use. */ if (ch->port == NULL || ch->port[0] == '\0') @@ -2350,8 +2352,8 @@ keep_going: /* We will come back to here until there is { case CHT_HOST_NAME: ret = pg_getaddrinfo_all(ch->host, portstr, &hint, - &conn->addrlist); - if (ret || !conn->addrlist) + &addrlist); + if (ret || !addrlist) { libpq_append_conn_error(conn, "could not translate host name \"%s\" to address: %s", ch->host, gai_strerror(ret)); @@ -2362,8 +2364,8 @@ keep_going: /* We will come back to here until there is case CHT_HOST_ADDRESS: hint.ai_flags = AI_NUMERICHOST; ret = pg_getaddrinfo_all(ch->hostaddr, portstr, &hint, - &conn->addrlist); - if (ret || !conn->addrlist) + &addrlist); + if (ret || !addrlist) { libpq_append_conn_error(conn, "could not parse network address \"%s\": %s", ch->hostaddr, gai_strerror(ret)); @@ -2372,7 +2374,7 @@ keep_going: /* We will come back to here until there is break; case CHT_UNIX_SOCKET: - conn->addrlist_family = hint.ai_family = AF_UNIX; + hint.ai_family = AF_UNIX; UNIXSOCK_PATH(portstr, thisport, ch->host); if (strlen(portstr) >= UNIXSOCK_PATH_BUFLEN) { @@ -2387,8 +2389,8 @@ keep_going: /* We will come back to here until there is * name as a Unix-domain socket path. */ ret = pg_getaddrinfo_all(NULL, portstr, &hint, - &conn->addrlist); - if (ret || !conn->addrlist) + &addrlist); + if (ret || !addrlist) { libpq_append_conn_error(conn, "could not translate Unix-domain socket path \"%s\" to address: %s", portstr, gai_strerror(ret)); @@ -2397,8 +2399,14 @@ keep_going: /* We will come back to here until there is break; } - /* OK, scan this addrlist for a working server address */ - conn->addr_cur = conn->addrlist; + if (!store_conn_addrinfo(conn, addrlist)) + { + pg_freeaddrinfo_all(hint.ai_family, addrlist); + libpq_append_conn_error(conn, "out of memory"); + goto error_return; + } + pg_freeaddrinfo_all(hint.ai_family, addrlist); + reset_connection_state_machine = true; conn->try_next_host = false; } @@ -2455,30 +2463,29 @@ keep_going: /* We will come back to here until there is { /* * Try to initiate a connection to one of the addresses - * returned by pg_getaddrinfo_all(). conn->addr_cur is the + * returned by pg_getaddrinfo_all(). conn->whichaddr is the * next one to try. * * The extra level of braces here is historical. It's not * worth reindenting this whole switch case to remove 'em. */ { - struct addrinfo *addr_cur = conn->addr_cur; char host_addr[NI_MAXHOST]; + AddrInfo *addr_cur; /* * Advance to next possible host, if we've tried all of * the addresses for the current host. */ - if (addr_cur == NULL) + if (conn->whichaddr == conn->naddr) { conn->try_next_host = true; goto keep_going; } + addr_cur = &conn->addr[conn->whichaddr]; /* Remember current address for possible use later */ - memcpy(&conn->raddr.addr, addr_cur->ai_addr, - addr_cur->ai_addrlen); - conn->raddr.salen = addr_cur->ai_addrlen; + memcpy(&conn->raddr, &addr_cur->addr, sizeof(SockAddr)); /* * Set connip, too. Note we purposely ignore strdup @@ -2494,7 +2501,7 @@ keep_going: /* We will come back to here until there is conn->connip = strdup(host_addr); /* Try to create the socket */ - conn->sock = socket(addr_cur->ai_family, SOCK_STREAM, 0); + conn->sock = socket(addr_cur->family, SOCK_STREAM, 0); if (conn->sock == PGINVALID_SOCKET) { int errorno = SOCK_ERRNO; @@ -2505,7 +2512,7 @@ keep_going: /* We will come back to here until there is * cases where the address list includes both IPv4 and * IPv6 but kernel only accepts one family. */ - if (addr_cur->ai_next != NULL || + if (conn->whichaddr < conn->naddr || conn->whichhost + 1 < conn->nconnhost) { conn->try_next_addr = true; @@ -2531,7 +2538,7 @@ keep_going: /* We will come back to here until there is * TCP sockets, nonblock mode, close-on-exec. Try the * next address if any of this fails. */ - if (addr_cur->ai_family != AF_UNIX) + if (addr_cur->family != AF_UNIX) { if (!connectNoDelay(conn)) { @@ -2558,7 +2565,7 @@ keep_going: /* We will come back to here until there is } #endif /* F_SETFD */ - if (addr_cur->ai_family != AF_UNIX) + if (addr_cur->family != AF_UNIX) { #ifndef WIN32 int on = 1; @@ -2650,8 +2657,8 @@ keep_going: /* We will come back to here until there is * Start/make connection. This should not block, since we * are in nonblock mode. If it does, well, too bad. */ - if (connect(conn->sock, addr_cur->ai_addr, - addr_cur->ai_addrlen) < 0) + if (connect(conn->sock, (struct sockaddr *) &addr_cur->addr.addr, + addr_cur->addr.salen) < 0) { if (SOCK_ERRNO == EINPROGRESS || #ifdef WIN32 @@ -4068,6 +4075,45 @@ freePGconn(PGconn *conn) free(conn); } +/* + * Copies over the addrinfos from addrlist to the PGconn. The reason we do this + * so that we can edit the resulting list as we please, because now the memory + * is owned by us. Changing the original addrinfo directly is risky, since we + * don't control how the memory is freed and by changing it we might confuse + * the implementation of freeaddrinfo. + */ +static bool +store_conn_addrinfo(PGconn *conn, struct addrinfo *addrlist) +{ + struct addrinfo *ai = addrlist; + + conn->whichaddr = 0; + + conn->naddr = 0; + while (ai) + { + ai = ai->ai_next; + conn->naddr++; + } + + conn->addr = calloc(conn->naddr, sizeof(AddrInfo)); + if (conn->addr == NULL) + return false; + + ai = addrlist; + for (int i = 0; i < conn->naddr; i++) + { + conn->addr[i].family = ai->ai_family; + + memcpy(&conn->addr[i].addr.addr, ai->ai_addr, + ai->ai_addrlen); + conn->addr[i].addr.salen = ai->ai_addrlen; + ai = ai->ai_next; + } + + return true; +} + /* * release_conn_addrinfo * - Free any addrinfo list in the PGconn. @@ -4075,11 +4121,10 @@ freePGconn(PGconn *conn) static void release_conn_addrinfo(PGconn *conn) { - if (conn->addrlist) + if (conn->addr) { - pg_freeaddrinfo_all(conn->addrlist_family, conn->addrlist); - conn->addrlist = NULL; - conn->addr_cur = NULL; /* for safety */ + free(conn->addr); + conn->addr = NULL; } } diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 85289980a11..4d40e8a2fbb 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -461,8 +461,10 @@ struct pg_conn PGTargetServerType target_server_type; /* desired session properties */ bool try_next_addr; /* time to advance to next address/host? */ bool try_next_host; /* time to advance to next connhost[]? */ - struct addrinfo *addrlist; /* list of addresses for current connhost */ - struct addrinfo *addr_cur; /* the one currently being tried */ + int naddr; /* number of addresses returned by getaddrinfo */ + int whichaddr; /* the address currently being tried */ + AddrInfo *addr; /* the array of addresses for the currently + * tried host */ int addrlist_family; /* needed to know how to free addrlist */ bool send_appname; /* okay to send application_name? */ diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 86a9303bf56..fa8881c9d93 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -26,6 +26,7 @@ AcquireSampleRowsFunc ActionList ActiveSnapshotElt AddForeignUpdateTargets_function +AddrInfo AffixNode AffixNodeData AfterTriggerEvent -- 2.34.1