v13-0006-libpq-Handle-NegotiateProtocolVersion-message-mo.patch

From c13922a3af5f238f5d61a33fc989a9641727ebbc Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <jelte.fennema@microsoft.com>
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