libpq: Handle NegotiateProtocolVersion message differently

Previously libpq would always error out if the server sends a
NegotiateProtocolVersion message. This was fine because libpq only
supported a single protocol version and did not support any protocol
parameters. But in the upcoming commits, we will introduce a new
protocol version and the NegotiateProtocolVersion message starts to
actually be used.

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. Currently this "range" only contains the
3.0 version, but in a future commit we'll change this.

Also clarify the error messages, making them suitable for the world
where libpq will support multiple protocol versions and protocol
extensions.

Note that until the later commits that introduce new protocol version,
this change does not have any behavioural effect, because libpq will
only request version 3.0 and will never send protocol parameters, and
therefore will never receive a NegotiateProtocolVersion message from
the server.

Author: Jelte Fennema-Nio <[email protected]>
Reviewed-by: Robert Haas <[email protected]> (earlier versions)
Discussion: https://www.postgresql.org/message-id/CAGECzQTfc_O%[email protected]
Discussion: https://www.postgresql.org/message-id/CAGECzQRbAGqJnnJJxTdKewTsNOovUt4bsx3NFfofz3m2j-t7tA@mail.gmail.com

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d5051f5e820f8905c7d2534411bfbbb3160cd661..0256753bd3eb8f8258ce01eee55702fdaf841754 100644 (file)
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -667,6 +667,7 @@ pqDropServerData(PGconn *conn)
 
    /* Reset assorted other per-connection state */
    conn->last_sqlstate[0] = '\0';
+   conn->pversion_negotiated = false;
    conn->auth_req_received = false;
    conn->client_finished_auth = false;
    conn->password_needed = false;
@@ -4084,16 +4085,24 @@ keep_going:                     /* We will come back to here until there is
 
                    CONNECTION_FAILED();
                }
+               /* Handle NegotiateProtocolVersion */
                else if (beresp == PqMsg_NegotiateProtocolVersion)
                {
+                   if (conn->pversion_negotiated)
+                   {
+                       libpq_append_conn_error(conn, "received duplicate protocol negotiation message");
+                       goto error_return;
+                   }
                    if (pqGetNegotiateProtocolVersion3(conn))
                    {
-                       libpq_append_conn_error(conn, "received invalid protocol negotiation message");
+                       /* pqGetNegotiateProtocolVersion3 set error already */
                        goto error_return;
                    }
+                   conn->pversion_negotiated = true;
+
                    /* OK, we read the message; mark data consumed */
                    pqParseDone(conn, conn->inCursor);
-                   goto error_return;
+                   goto keep_going;
                }
 
                /* It is an authentication request. */

diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 95dd456f076476ff39882e688ea4af11a616fb49..43e3519e4bd9a242f5349bb3622d20990d2c1eb1 100644 (file)
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -870,6 +870,7 @@ advance_and_error:
 /*
  * Attempt to read an Error or Notice response message.
  * This is possible in several places, so we break it out as a subroutine.
+ *
  * Entry: 'E' or 'N' message type and length have already been consumed.
  * Exit: returns 0 if successfully consumed message.
  *      returns EOF if not enough data.
@@ -1399,64 +1400,87 @@ reportErrorPosition(PQExpBuffer msg, const char *query, int loc, int encoding)
 
 
 /*
- * Attempt to read a NegotiateProtocolVersion message.
+ * Attempt to read a NegotiateProtocolVersion message.  Sets conn->pversion
+ * to the version that's negotiated by the server.
+ *
  * Entry: 'v' message type and length have already been consumed.
  * Exit: returns 0 if successfully consumed message.
- *      returns EOF if not enough data.
+ *      returns 1 on failure. The error message is filled in.
  */
 int
 pqGetNegotiateProtocolVersion3(PGconn *conn)
 {
-   int         tmp;
-   ProtocolVersion their_version;
+   int         their_version;
    int         num;
-   PQExpBufferData buf;
 
-   if (pqGetInt(&tmp, 4, conn) != 0)
-       return EOF;
-   their_version = tmp;
+   if (pqGetInt(&their_version, 4, conn) != 0)
+       goto eof;
 
    if (pqGetInt(&num, 4, conn) != 0)
-       return EOF;
+       goto eof;
 
-   initPQExpBuffer(&buf);
-   for (int i = 0; i < num; i++)
+   /* Check the protocol version */
+   if (their_version > conn->pversion)
    {
-       if (pqGets(&conn->workBuffer, conn))
-       {
-           termPQExpBuffer(&buf);
-           return EOF;
-       }
-       if (buf.len > 0)
-           appendPQExpBufferChar(&buf, ' ');
-       appendPQExpBufferStr(&buf, conn->workBuffer.data);
+       libpq_append_conn_error(conn, "received invalid protocol negotiation message: server requested downgrade to a higher-numbered version");
+       goto failure;
+   }
+
+   if (their_version < PG_PROTOCOL(3, 0))
+   {
+       libpq_append_conn_error(conn, "received invalid protocol negotiation message: server requested downgrade to pre-3.0 protocol version");
+       goto failure;
    }
 
-   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)
+   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');
+       libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported negative number of unsupported parameters");
+       goto failure;
+   }
+
+   if (their_version == conn->pversion && num == 0)
+   {
+       libpq_append_conn_error(conn, "received invalid protocol negotiation message: server negotiated but asks for no changes");
+       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");
+   /* the version is acceptable */
+   conn->pversion = their_version;
+
+   /*
+    * We don't currently request any protocol extensions, so we don't expect
+    * the server to reply with any either.
+    */
+   for (int i = 0; i < num; i++)
+   {
+       if (pqGets(&conn->workBuffer, conn))
+       {
+           goto eof;
+       }
+       if (strncmp(conn->workBuffer.data, "_pq_.", 5) != 0)
+       {
+           libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported unsupported parameter name without a _pq_. prefix (\"%s\")", conn->workBuffer.data);
+           goto failure;
+       }
+       libpq_append_conn_error(conn, "received invalid protocol negotiation message: server reported an unsupported parameter that was not requested (\"%s\")", conn->workBuffer.data);
+       goto failure;
+   }
 
-   termPQExpBuffer(&buf);
    return 0;
+
+eof:
+   libpq_append_conn_error(conn, "received invalid protocol negotation message: message too short");
+failure:
+   conn->asyncStatus = PGASYNC_READY;
+   pqSaveErrorResult(conn);
+   return 1;
 }
 
 
 /*
  * Attempt to read a ParameterStatus message.
  * This is possible in several places, so we break it out as a subroutine.
+ *
  * Entry: 'S' message type and length have already been consumed.
  * Exit: returns 0 if successfully consumed message.
  *      returns EOF if not enough data.
@@ -1486,6 +1510,7 @@ getParameterStatus(PGconn *conn)
 /*
  * Attempt to read a Notify response message.
  * This is possible in several places, so we break it out as a subroutine.
+ *
  * Entry: 'A' message type and length have already been consumed.
  * Exit: returns 0 if successfully consumed Notify message.
  *      returns EOF if not enough data.

diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index ade5ad82f07cf29697254835474c828a5624dcd7..d9f2e9ad74377c2675c557fbcd0553d2b9d13c6c 100644 (file)
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -496,6 +496,8 @@ struct pg_conn
    SockAddr    raddr;          /* Remote address */
    ProtocolVersion pversion;   /* FE/BE protocol version in use */
    int         sversion;       /* server version, e.g. 70401 for 7.4.1 */
+   bool        pversion_negotiated;    /* true if NegotiateProtocolVersion
+                                        * was received */
    bool        auth_req_received;  /* true if any type of auth req received */
    bool        password_needed;    /* true if server demanded a password */
    bool        gssapi_used;    /* true if authenticated via gssapi */