From c13922a3af5f238f5d61a33fc989a9641727ebbc Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Tue, 2 Jan 2024 11:16:19 +0100 Subject: [PATCH v13 6/9] libpq: Handle NegotiateProtocolVersion message more leniently Previously libpq would always error when the server returns a NegotiateProtocolVersion message. This was fine because libpq only supported a single protocol version and did not support any protocol parameters. But we now that we're discussing protocol changes we need to change this behaviour, and introduce a fallback mechanism when connecting to an older server. This patch modifies the client side checks to allow a range of supported protocol versions, instead of only allowing the exact version that was requested. In addition it now allows connecting when the server does not support some of the requested protocol parameters. Note that at the moment this change does not have any behavioural effect, because libpq will only request version 3.0 and will never send protocol parameters. Which means that the client never receives a NegotiateProtocolVersion message from the server. --- src/interfaces/libpq/fe-connect.c | 48 +++++++++++-- src/interfaces/libpq/fe-protocol3.c | 108 +++++++++++++++++++++------- src/interfaces/libpq/libpq-int.h | 18 +++++ src/tools/pgindent/typedefs.list | 1 + 4 files changed, 144 insertions(+), 31 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 7c75e4c46da..694becbdc4c 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -1884,11 +1884,30 @@ pqConnectOptions2(PGconn *conn) else { /* - * To not break connecting to older servers/poolers that do not yet - * support NegotiateProtocolVersion, default to the 3.0 protocol at - * least for a while longer. + * Since some old servers and poolers don't support the + * NegotiateProtocolVersion message. So we only want to send a + * StartupMessage that contains protocol parameters or a non-3.0 + * protocol version by default. Since either of those would cause a + * failure to connect to an old server. But as soon as the provided + * connection string requires a protocol parameter, we might as well + * ask for the latest protocol version we support by default too. */ - conn->max_pversion = PG_PROTOCOL(3, 0); + bool needs_new_protocol_features = false; + + for (const pg_protocol_parameter *param = KnownProtocolParameters; param->name; param++) + { + const char *value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + + if (value && value[0]) + { + needs_new_protocol_features = true; + break; + } + } + if (needs_new_protocol_features) + conn->max_pversion = PG_PROTOCOL_LATEST; + else + conn->max_pversion = PG_PROTOCOL(3, 0); } /* @@ -3915,12 +3934,20 @@ keep_going: /* We will come back to here until there is goto error_return; } + if (conn->pversion < PG_PROTOCOL_EARLIEST) + { + libpq_append_conn_error(conn, "protocol version not supported by server: client supports down to %u.%u, server supports up to %u.%u", + PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST), PG_PROTOCOL_MINOR(PG_PROTOCOL_EARLIEST), + PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion)); + goto error_return; + } + if (conn->Pfdebug) pqTraceOutputMessage(conn, conn->inBuffer + conn->inStart, false); /* OK, we read the message; mark data consumed */ conn->inStart = conn->inCursor; - goto error_return; + goto keep_going; } /* It is an authentication request. */ @@ -4715,6 +4742,17 @@ freePGconn(PGconn *conn) free(conn->events[i].name); } + for (const pg_protocol_parameter *param = KnownProtocolParameters; param->name; param++) + { + char *conn_string_value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + char *server_value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + char *supported_value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + + free(conn_string_value); + free(server_value); + free(supported_value); + } + release_conn_addrinfo(conn); pqReleaseConnHosts(conn); diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index 3170d484f02..5e200519190 100644 --- a/src/interfaces/libpq/fe-protocol3.c +++ b/src/interfaces/libpq/fe-protocol3.c @@ -56,6 +56,9 @@ static void reportErrorPosition(PQExpBuffer msg, const char *query, static int build_startup_packet(const PGconn *conn, char *packet, const PQEnvironmentOption *options); +const struct pg_protocol_parameter KnownProtocolParameters[] = { + {NULL, NULL, 0, 0} +}; /* * parseInput: if appropriate, parse input data from backend @@ -1411,49 +1414,74 @@ reportErrorPosition(PQExpBuffer msg, const char *query, int loc, int encoding) int pqGetNegotiateProtocolVersion3(PGconn *conn) { - int tmp; - ProtocolVersion their_version; + int their_version; int num; - PQExpBufferData buf; - if (pqGetInt(&tmp, 4, conn) != 0) + if (pqGetInt(&their_version, 4, conn) != 0) return EOF; - their_version = tmp; if (pqGetInt(&num, 4, conn) != 0) return EOF; - initPQExpBuffer(&buf); + /* these should never happen */ + if (their_version > conn->pversion) + { + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion message, server version is newer than client version"); + goto failure; + } + + if (num < 0) + { + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion message, negative protocol parameter count"); + goto failure; + } + + /* it's reporting "all good", so server shouldn't have sent it */ + if (their_version == conn->pversion && num == 0) + { + libpq_append_conn_error(conn, "unexpected NegotiateProtocolVersion message, server supports requested features"); + goto failure; + } + + conn->pversion = their_version; for (int i = 0; i < num; i++) { + bool found = false; + if (pqGets(&conn->workBuffer, conn)) { - termPQExpBuffer(&buf); return EOF; } - if (buf.len > 0) - appendPQExpBufferChar(&buf, ' '); - appendPQExpBufferStr(&buf, conn->workBuffer.data); - } - if (their_version < conn->pversion) - libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u", - PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion), - PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version)); - if (num > 0) - { - appendPQExpBuffer(&conn->errorMessage, - libpq_ngettext("protocol extension not supported by server: %s", - "protocol extensions not supported by server: %s", num), - buf.data); - appendPQExpBufferChar(&conn->errorMessage, '\n'); + for (const pg_protocol_parameter *param = KnownProtocolParameters; param->name; param++) + { + if (strncmp(conn->workBuffer.data, "_pq_.", 5) != 0) + { + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion message, server reported parameter without _pq_. prefix %s", conn->workBuffer.data); + goto failure; + } + if (strcmp(param->name, &conn->workBuffer.data[5]) == 0) + { + char **server_value = (char **) ((char *) conn + param->conn_server_value_offset); + + *server_value = NULL; + found = true; + break; + } + } + + if (!found) + { + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion message, server reported unknown unsupported parameter %s", conn->workBuffer.data); + goto failure; + } } - /* neither -- server shouldn't have sent it */ - if (!(their_version < conn->pversion) && !(num > 0)) - libpq_append_conn_error(conn, "invalid %s message", "NegotiateProtocolVersion"); + return 0; - termPQExpBuffer(&buf); +failure: + conn->asyncStatus = PGASYNC_READY; + pqSaveErrorResult(conn); return 0; } @@ -2314,6 +2342,34 @@ build_startup_packet(const PGconn *conn, char *packet, } } + /* + * If we are requesting protocol version that's higher than 3.0, also + * request all known protocol parameters. That way, we can know which + * parameters are supported by the server. We don't request any parameters + * for older protocol versions, because not all old servers support the + * negotiation mechanism that newer servers support. + */ + if (conn->max_pversion > PG_PROTOCOL(3, 0)) + { + for (const pg_protocol_parameter *param = KnownProtocolParameters; param->name; param++) + { + const char *value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + + if (!value || !value[0]) + value = param->default_value; + + /* + * Add the _pq_. prefix to the parameter name. This is needed for + * all protocol parameters. + */ + if (packet) + strcpy(packet + packet_len, "_pq_."); + packet_len += 5; + + ADD_STARTUP_OPTION(param->name, value); + } + } + /* Add trailing terminator */ if (packet) packet[packet_len] = '\0'; diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index f7f4eca11d9..8b8560268f8 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -639,6 +639,24 @@ struct pg_conn PQExpBufferData workBuffer; /* expansible string */ }; +typedef struct pg_protocol_parameter +{ + const char *name; + const char *default_value; + + /* + * Offset of the "connection string value" string in the connection + * structure + */ + off_t conn_connection_string_value_offset; + /* Offset of the "server value" in the connection structure */ + off_t conn_server_value_offset; + /* Offset of the "server support string" in the connection structure */ + off_t conn_server_support_offset; +} pg_protocol_parameter; + +extern const struct pg_protocol_parameter KnownProtocolParameters[]; + /* String descriptions of the ExecStatusTypes. * direct use of this array is deprecated; call PQresStatus() instead. diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index d427a1c16a5..97b4e5f1e02 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3695,6 +3695,7 @@ pg_mb_radix_tree pg_md5_ctx pg_on_exit_callback pg_prng_state +pg_protocol_parameter pg_re_flags pg_saslprep_rc pg_sha1_ctx -- 2.34.1