From c0af30b47db023b87c4eb6b55190a8c081697241 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri, 7 Sep 2012 16:02:23 -0400
Subject: [PATCH 1/4] Centralize libpq's low-level code for dropping a
connection.
Create an internal function pqDropConnection that does the physical socket
close and cleans up closely-associated state. This removes a bunch of ad
hoc, not always consistent closure code. The ulterior motive is to have a
single place to wait for a spawned child backend to exit, but this seems
like good cleanup even if that never happens.
I went back and forth on whether to include "conn->status = CONNECTION_BAD"
in pqDropConnection's actions, but for the moment decided not to. Only a
minority of the call sites actually want that, and in any case it's
arguable that conn->status is slightly higher-level state, and thus not
part of this function's purview.
Upstream commit: 210eb9b743c0645df05e5c8be4490ba4f09fc871
---
src/interfaces/libpq/fe-connect.c | 95 +++++++++++------------------
src/interfaces/libpq/fe-misc.c | 5 +-
src/interfaces/libpq/fe-protocol3.c | 4 +-
src/interfaces/libpq/libpq-int.h | 1 +
4 files changed, 38 insertions(+), 67 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 28b96a6..6985118 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -363,6 +363,28 @@ static void default_threadlock(int acquire);
pgthreadlock_t pg_g_threadlock = default_threadlock;
+/*
+ * pqDropConnection
+ *
+ * Close any physical connection to the server, and reset associated
+ * state inside the connection object. We don't release state that
+ * would be needed to reconnect, though.
+ */
+void
+pqDropConnection(PGconn *conn)
+{
+ /* Drop any SSL state */
+ pqsecure_close(conn);
+ /* Close the socket itself */
+ if (conn->sock >= 0)
+ closesocket(conn->sock);
+ conn->sock = -1;
+ /* Discard any unread/unsent data */
+ conn->inStart = conn->inCursor = conn->inEnd = 0;
+ conn->outCount = 0;
+}
+
+
/*
* Connecting to a Database
*
@@ -1477,12 +1499,7 @@ connectDBStart(PGconn *conn)
return 1;
connect_errReturn:
- if (conn->sock >= 0)
- {
- pqsecure_close(conn);
- closesocket(conn->sock);
- conn->sock = -1;
- }
+ pqDropConnection(conn);
conn->status = CONNECTION_BAD;
return 0;
}
@@ -1720,8 +1737,7 @@ keep_going: /* We will come back to here until there is
{
if (!connectNoDelay(conn))
{
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1731,8 +1747,7 @@ keep_going: /* We will come back to here until there is
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to non-blocking mode: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1743,8 +1758,7 @@ keep_going: /* We will come back to here until there is
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to close-on-exec mode: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1792,8 +1806,7 @@ keep_going: /* We will come back to here until there is
if (err)
{
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1880,11 +1893,7 @@ keep_going: /* We will come back to here until there is
* failure and keep going if there are more addresses.
*/
connectFailureMessage(conn, SOCK_ERRNO);
- if (conn->sock >= 0)
- {
- closesocket(conn->sock);
- conn->sock = -1;
- }
+ pqDropConnection(conn);
/*
* Try the next address, if any.
@@ -1929,6 +1938,7 @@ keep_going: /* We will come back to here until there is
* error message.
*/
connectFailureMessage(conn, optval);
+ pqDropConnection(conn);
/*
* If more addresses remain, keep trying, just as in the
@@ -1936,11 +1946,6 @@ keep_going: /* We will come back to here until there is
*/
if (conn->addr_cur->ai_next != NULL)
{
- if (conn->sock >= 0)
- {
- closesocket(conn->sock);
- conn->sock = -1;
- }
conn->addr_cur = conn->addr_cur->ai_next;
conn->status = CONNECTION_NEEDED;
goto keep_going;
@@ -2215,12 +2220,8 @@ keep_going: /* We will come back to here until there is
/* only retry once */
conn->allow_ssl_try = false;
/* Must drop the old connection */
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_NEEDED;
- /* Discard any unread/unsent data */
- conn->inStart = conn->inCursor = conn->inEnd = 0;
- conn->outCount = 0;
goto keep_going;
}
}
@@ -2330,13 +2331,8 @@ keep_going: /* We will come back to here until there is
{
conn->pversion = PG_PROTOCOL(2, 0);
/* Must drop the old connection */
- pqsecure_close(conn);
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_NEEDED;
- /* Discard any unread/unsent data */
- conn->inStart = conn->inCursor = conn->inEnd = 0;
- conn->outCount = 0;
goto keep_going;
}
@@ -2401,12 +2397,8 @@ keep_going: /* We will come back to here until there is
/* only retry once */
conn->wait_ssl_try = false;
/* Must drop the old connection */
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_NEEDED;
- /* Discard any unread/unsent data */
- conn->inStart = conn->inCursor = conn->inEnd = 0;
- conn->outCount = 0;
goto keep_going;
}
@@ -2421,13 +2413,8 @@ keep_going: /* We will come back to here until there is
/* only retry once */
conn->allow_ssl_try = false;
/* Must drop the old connection */
- pqsecure_close(conn);
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_NEEDED;
- /* Discard any unread/unsent data */
- conn->inStart = conn->inCursor = conn->inEnd = 0;
- conn->outCount = 0;
goto keep_going;
}
#endif
@@ -2587,13 +2574,8 @@ keep_going: /* We will come back to here until there is
PQclear(res);
conn->send_appname = false;
/* Must drop the old connection */
- pqsecure_close(conn);
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_NEEDED;
- /* Discard any unread/unsent data */
- conn->inStart = conn->inCursor = conn->inEnd = 0;
- conn->outCount = 0;
goto keep_going;
}
}
@@ -2987,12 +2969,7 @@ closePGconn(PGconn *conn)
/*
* Close the connection, reset all transient state, flush I/O buffers.
*/
- if (conn->sock >= 0)
- {
- pqsecure_close(conn);
- closesocket(conn->sock);
- }
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_BAD; /* Well, not really _bad_ - just
* absent */
conn->asyncStatus = PGASYNC_IDLE;
@@ -3022,8 +2999,6 @@ closePGconn(PGconn *conn)
if (conn->lobjfuncs)
free(conn->lobjfuncs);
conn->lobjfuncs = NULL;
- conn->inStart = conn->inCursor = conn->inEnd = 0;
- conn->outCount = 0;
#ifdef ENABLE_GSS
{
OM_uint32 min_s;
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 8b0d8ef..c1c5c75 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -815,11 +815,8 @@ definitelyEOF:
/* Come here if lower-level code already set a suitable errorMessage */
definitelyFailed:
+ pqDropConnection(conn);
conn->status = CONNECTION_BAD; /* No more connection to backend */
- pqsecure_close(conn);
- closesocket(conn->sock);
- conn->sock = -1;
-
return -1;
}
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index f997257..b130b4c 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -457,9 +457,7 @@ handleSyncLoss(PGconn *conn, char id, int msgLength)
pqSaveErrorResult(conn);
conn->asyncStatus = PGASYNC_READY; /* drop out of GetResult wait loop */
- pqsecure_close(conn);
- closesocket(conn->sock);
- conn->sock = -1;
+ pqDropConnection(conn);
conn->status = CONNECTION_BAD; /* No more connection to backend */
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index bc2be3c..70b956b 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -489,6 +489,7 @@ extern char *const pgresStatus[];
/* === in fe-connect.c === */
+extern void pqDropConnection(PGconn *conn);
extern int pqPacketSend(PGconn *conn, char pack_type,
const void *buf, size_t buf_len);
extern bool pqGetHomeDirectory(char *buf, int bufsize);
--
2.17.1
From 299e76cdeebf150f51cf29fa4269ae02cf6e7f24 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 12 Nov 2015 13:03:53 -0500
Subject: [PATCH 2/4] Fix unwanted flushing of libpq's input buffer when socket
EOF is seen.
In commit 210eb9b743c0645d I centralized libpq's logic for closing down
the backend communication socket, and made the new pqDropConnection
routine always reset the I/O buffers to empty. Many of the call sites
previously had not had such code, and while that amounted to an oversight
in some cases, there was one place where it was intentional and necessary
*not* to flush the input buffer: pqReadData should never cause that to
happen, since we probably still want to process whatever data we read.
This is the true cause of the problem Robert was attempting to fix in
c3e7c24a1d60dc6a, namely that libpq no longer reported the backend's final
ERROR message before reporting "server closed the connection unexpectedly".
But that only accidentally fixed it, by invoking parseInput before the
input buffer got flushed; and very likely there are timing scenarios
where we'd still lose the message before processing it.
To fix, pass a flag to pqDropConnection to tell it whether to flush the
input buffer or not. On review I think flushing is actually correct for
every other call site.
Back-patch to 9.3 where the problem was introduced. In HEAD, also improve
the comments added by c3e7c24a1d60dc6a.
Upstream commit: db6e8e1624a8f0357373450136c850f2b6e7fc8a
---
src/interfaces/libpq/fe-connect.c | 38 +++++++++++++++++------------
src/interfaces/libpq/fe-misc.c | 3 ++-
src/interfaces/libpq/fe-protocol3.c | 4 +--
src/interfaces/libpq/libpq-int.h | 2 +-
4 files changed, 27 insertions(+), 20 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 6985118..c22901d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -369,9 +369,13 @@ pgthreadlock_t pg_g_threadlock = default_threadlock;
* Close any physical connection to the server, and reset associated
* state inside the connection object. We don't release state that
* would be needed to reconnect, though.
+ *
+ * We can always flush the output buffer, since there's no longer any hope
+ * of sending that data. However, unprocessed input data might still be
+ * valuable, so the caller must tell us whether to flush that or not.
*/
void
-pqDropConnection(PGconn *conn)
+pqDropConnection(PGconn *conn, bool flushInput)
{
/* Drop any SSL state */
pqsecure_close(conn);
@@ -379,8 +383,10 @@ pqDropConnection(PGconn *conn)
if (conn->sock >= 0)
closesocket(conn->sock);
conn->sock = -1;
- /* Discard any unread/unsent data */
- conn->inStart = conn->inCursor = conn->inEnd = 0;
+ /* Optionally discard any unread data */
+ if (flushInput)
+ conn->inStart = conn->inCursor = conn->inEnd = 0;
+ /* Always discard any unsent data */
conn->outCount = 0;
}
@@ -1499,7 +1505,7 @@ connectDBStart(PGconn *conn)
return 1;
connect_errReturn:
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_BAD;
return 0;
}
@@ -1737,7 +1743,7 @@ keep_going: /* We will come back to here until there is
{
if (!connectNoDelay(conn))
{
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1747,7 +1753,7 @@ keep_going: /* We will come back to here until there is
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to non-blocking mode: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1758,7 +1764,7 @@ keep_going: /* We will come back to here until there is
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to close-on-exec mode: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1806,7 +1812,7 @@ keep_going: /* We will come back to here until there is
if (err)
{
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->addr_cur = addr_cur->ai_next;
continue;
}
@@ -1893,7 +1899,7 @@ keep_going: /* We will come back to here until there is
* failure and keep going if there are more addresses.
*/
connectFailureMessage(conn, SOCK_ERRNO);
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
/*
* Try the next address, if any.
@@ -1938,7 +1944,7 @@ keep_going: /* We will come back to here until there is
* error message.
*/
connectFailureMessage(conn, optval);
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
/*
* If more addresses remain, keep trying, just as in the
@@ -2220,7 +2226,7 @@ keep_going: /* We will come back to here until there is
/* only retry once */
conn->allow_ssl_try = false;
/* Must drop the old connection */
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_NEEDED;
goto keep_going;
}
@@ -2331,7 +2337,7 @@ keep_going: /* We will come back to here until there is
{
conn->pversion = PG_PROTOCOL(2, 0);
/* Must drop the old connection */
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_NEEDED;
goto keep_going;
}
@@ -2397,7 +2403,7 @@ keep_going: /* We will come back to here until there is
/* only retry once */
conn->wait_ssl_try = false;
/* Must drop the old connection */
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_NEEDED;
goto keep_going;
}
@@ -2413,7 +2419,7 @@ keep_going: /* We will come back to here until there is
/* only retry once */
conn->allow_ssl_try = false;
/* Must drop the old connection */
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_NEEDED;
goto keep_going;
}
@@ -2574,7 +2580,7 @@ keep_going: /* We will come back to here until there is
PQclear(res);
conn->send_appname = false;
/* Must drop the old connection */
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_NEEDED;
goto keep_going;
}
@@ -2969,7 +2975,7 @@ closePGconn(PGconn *conn)
/*
* Close the connection, reset all transient state, flush I/O buffers.
*/
- pqDropConnection(conn);
+ pqDropConnection(conn, true);
conn->status = CONNECTION_BAD; /* Well, not really _bad_ - just
* absent */
conn->asyncStatus = PGASYNC_IDLE;
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index c1c5c75..58c9ce0 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -815,7 +815,8 @@ definitelyEOF:
/* Come here if lower-level code already set a suitable errorMessage */
definitelyFailed:
- pqDropConnection(conn);
+ /* Do *not* drop any already-read data; caller still wants it */
+ pqDropConnection(conn, false);
conn->status = CONNECTION_BAD; /* No more connection to backend */
return -1;
}
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index b130b4c..88ff74a 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -456,8 +456,8 @@ handleSyncLoss(PGconn *conn, char id, int msgLength)
/* build an error result holding the error message */
pqSaveErrorResult(conn);
conn->asyncStatus = PGASYNC_READY; /* drop out of GetResult wait loop */
-
- pqDropConnection(conn);
+ /* flush input data since we're giving up on processing it */
+ pqDropConnection(conn, true);
conn->status = CONNECTION_BAD; /* No more connection to backend */
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 70b956b..8843ccb 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -489,7 +489,7 @@ extern char *const pgresStatus[];
/* === in fe-connect.c === */
-extern void pqDropConnection(PGconn *conn);
+extern void pqDropConnection(PGconn *conn, bool flushInput);
extern int pqPacketSend(PGconn *conn, char pack_type,
const void *buf, size_t buf_len);
extern bool pqGetHomeDirectory(char *buf, int bufsize);
--
2.17.1
From 3b08b525a118be43a334045409b1bad9cfaeb438 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Wed, 7 Jun 2017 14:01:46 +0300
Subject: [PATCH 3/4] Clear auth context correctly when re-connecting after
failed auth attempt.
If authentication over an SSL connection fails, with sslmode=prefer,
libpq will reconnect without SSL and retry. However, we did not clear
the variables related to GSS, SSPI, and SASL authentication state, when
reconnecting. Because of that, the second authentication attempt would
always fail with a "duplicate GSS/SASL authentication request" error.
pg_SSPI_startup did not check for duplicate authentication requests like
the corresponding GSS and SASL functions, so with SSPI, you would leak
some memory instead.
Another way this could manifest itself, on version 10, is if you list
multiple hostnames in the "host" parameter. If the first server requests
Kerberos or SCRAM authentication, but it fails, the attempts to connect to
the other servers will also fail with "duplicate authentication request"
errors.
To fix, move the clearing of authentication state from closePGconn to
pgDropConnection, so that it is cleared also when re-connecting.
Patch by Michael Paquier, with some kibitzing by me.
Backpatch down to 9.3. 9.2 has the same bug, but the code around closing
the connection is somewhat different, so that this patch doesn't apply.
To fix this in 9.2, I think we would need to back-port commit 210eb9b743
first, and then apply this patch. However, given that we only bumped into
this in our own testing, we haven't heard any reports from users about
this, and that 9.2 will be end-of-lifed in a couple of months anyway, it
doesn't seem worth the risk and trouble.
Discussion: https://www.postgresql.org/message-id/CAB7nPqRuOUm0MyJaUy9L3eXYJU3AKCZ-0-03=-aDTZJGV4GyWw@mail.gmail.com
Upstream commit: f2fa0c6514b6c5b7bccfe5050f6791dea1113c2e
---
src/interfaces/libpq/fe-auth.c | 7 ++-
src/interfaces/libpq/fe-connect.c | 76 +++++++++++++++++--------------
2 files changed, 47 insertions(+), 36 deletions(-)
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index d10bd9f..ee961d9 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -618,7 +618,12 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate)
SECURITY_STATUS r;
TimeStamp expire;
- conn->sspictx = NULL;
+ if (conn->sspictx)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("duplicate SSPI authentication request\n"));
+ return STATUS_ERROR;
+ }
/*
* Retreive credentials handle
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c22901d..6dbcbc6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -379,15 +379,56 @@ pqDropConnection(PGconn *conn, bool flushInput)
{
/* Drop any SSL state */
pqsecure_close(conn);
+
/* Close the socket itself */
if (conn->sock >= 0)
closesocket(conn->sock);
conn->sock = -1;
+
/* Optionally discard any unread data */
if (flushInput)
conn->inStart = conn->inCursor = conn->inEnd = 0;
+
/* Always discard any unsent data */
conn->outCount = 0;
+
+ /* Free authentication state */
+#ifdef ENABLE_GSS
+ {
+ OM_uint32 min_s;
+
+ if (conn->gctx)
+ gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
+ if (conn->gtarg_nam)
+ gss_release_name(&min_s, &conn->gtarg_nam);
+ if (conn->ginbuf.length)
+ gss_release_buffer(&min_s, &conn->ginbuf);
+ if (conn->goutbuf.length)
+ gss_release_buffer(&min_s, &conn->goutbuf);
+ }
+#endif
+#ifdef ENABLE_SSPI
+ if (conn->ginbuf.length)
+ free(conn->ginbuf.value);
+ conn->ginbuf.length = 0;
+ conn->ginbuf.value = NULL;
+ if (conn->sspitarget)
+ free(conn->sspitarget);
+ conn->sspitarget = NULL;
+ if (conn->sspicred)
+ {
+ FreeCredentialsHandle(conn->sspicred);
+ free(conn->sspicred);
+ conn->sspicred = NULL;
+ }
+ if (conn->sspictx)
+ {
+ DeleteSecurityContext(conn->sspictx);
+ free(conn->sspictx);
+ conn->sspictx = NULL;
+ }
+ conn->usesspi = 0;
+#endif
}
@@ -3005,41 +3046,6 @@ closePGconn(PGconn *conn)
if (conn->lobjfuncs)
free(conn->lobjfuncs);
conn->lobjfuncs = NULL;
-#ifdef ENABLE_GSS
- {
- OM_uint32 min_s;
-
- if (conn->gctx)
- gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
- if (conn->gtarg_nam)
- gss_release_name(&min_s, &conn->gtarg_nam);
- if (conn->ginbuf.length)
- gss_release_buffer(&min_s, &conn->ginbuf);
- if (conn->goutbuf.length)
- gss_release_buffer(&min_s, &conn->goutbuf);
- }
-#endif
-#ifdef ENABLE_SSPI
- if (conn->ginbuf.length)
- free(conn->ginbuf.value);
- conn->ginbuf.length = 0;
- conn->ginbuf.value = NULL;
- if (conn->sspitarget)
- free(conn->sspitarget);
- conn->sspitarget = NULL;
- if (conn->sspicred)
- {
- FreeCredentialsHandle(conn->sspicred);
- free(conn->sspicred);
- conn->sspicred = NULL;
- }
- if (conn->sspictx)
- {
- DeleteSecurityContext(conn->sspictx);
- free(conn->sspictx);
- conn->sspictx = NULL;
- }
-#endif
}
/*
--
2.17.1
From f25aa65f201df8925c39373aa10dfee19253d03e Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 6 Aug 2018 10:53:35 -0400
Subject: [PATCH 4/4] Fix failure to reset libpq's state fully between
connection attempts.
The logic in PQconnectPoll() did not take care to ensure that all of
a PGconn's internal state variables were reset before trying a new
connection attempt. If we got far enough in the connection sequence
to have changed any of these variables, and then decided to try a new
server address or server name, the new connection might be completed
with some state that really only applied to the failed connection.
While this has assorted bad consequences, the only one that is clearly
a security issue is that password_needed didn't get reset, so that
if the first server asked for a password and the second didn't,
PQconnectionUsedPassword() would return an incorrect result. This
could be leveraged by unprivileged users of dblink or postgres_fdw
to allow them to use server-side login credentials that they should
not be able to use.
Other notable problems include the possibility of forcing a v2-protocol
connection to a server capable of supporting v3, or overriding
"sslmode=prefer" to cause a non-encrypted connection to a server that
would have accepted an encrypted one. Those are certainly bugs but
it's harder to paint them as security problems in themselves. However,
forcing a v2-protocol connection could result in libpq having a wrong
idea of the server's standard_conforming_strings setting, which opens
the door to SQL-injection attacks. The extent to which that's actually
a problem, given the prerequisite that the attacker needs control of
the client's connection parameters, is unclear.
These problems have existed for a long time, but became more easily
exploitable in v10, both because it introduced easy ways to force libpq
to abandon a connection attempt at a late stage and then try another one
(rather than just giving up), and because it provided an easy way to
specify multiple target hosts.
Fix by rearranging PQconnectPoll's state machine to provide centralized
places to reset state properly when moving to a new target host or when
dropping and retrying a connection to the same host.
Tom Lane, reviewed by Noah Misch. Our thanks to Andrew Krasichkov
for finding and reporting the problem.
Security: CVE-2018-10915
Upstream commit: 243de06be96d6001d01f2ec7c4573aad8b657195
---
src/interfaces/libpq/fe-connect.c | 301 +++++++++++++++++++-----------
src/interfaces/libpq/libpq-int.h | 2 +
2 files changed, 196 insertions(+), 107 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 6dbcbc6..ca06337 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -368,7 +368,8 @@ pgthreadlock_t pg_g_threadlock = default_threadlock;
*
* Close any physical connection to the server, and reset associated
* state inside the connection object. We don't release state that
- * would be needed to reconnect, though.
+ * would be needed to reconnect, though, nor local state that might still
+ * be useful later.
*
* We can always flush the output buffer, since there's no longer any hope
* of sending that data. However, unprocessed input data might still be
@@ -432,6 +433,64 @@ pqDropConnection(PGconn *conn, bool flushInput)
}
+/*
+ * pqDropServerData
+ *
+ * Clear all connection state data that was received from (or deduced about)
+ * the server. This is essential to do between connection attempts to
+ * different servers, else we may incorrectly hold over some data from the
+ * old server.
+ *
+ * It would be better to merge this into pqDropConnection, perhaps, but
+ * right now we cannot because that function is called immediately on
+ * detection of connection loss (cf. pqReadData, for instance). This data
+ * should be kept until we are actually starting a new connection.
+ */
+static void
+pqDropServerData(PGconn *conn)
+{
+ PGnotify *notify;
+ pgParameterStatus *pstatus;
+
+ /* Forget pending notifies */
+ notify = conn->notifyHead;
+ while (notify != NULL)
+ {
+ PGnotify *prev = notify;
+
+ notify = notify->next;
+ free(prev);
+ }
+ conn->notifyHead = conn->notifyTail = NULL;
+
+ /* Reset ParameterStatus data, as well as variables deduced from it */
+ pstatus = conn->pstatus;
+ while (pstatus != NULL)
+ {
+ pgParameterStatus *prev = pstatus;
+
+ pstatus = pstatus->next;
+ free(prev);
+ }
+ conn->pstatus = NULL;
+ conn->client_encoding = PG_SQL_ASCII;
+ conn->std_strings = false;
+ conn->sversion = 0;
+
+ /* Drop large-object lookup data */
+ if (conn->lobjfuncs)
+ free(conn->lobjfuncs);
+ conn->lobjfuncs = NULL;
+
+ /* Reset assorted other per-connection state */
+ conn->last_sqlstate[0] = '\0';
+ conn->auth_req_received = false;
+ conn->password_needed = false;
+ conn->be_pid = 0;
+ conn->be_key = 0;
+}
+
+
/*
* Connecting to a Database
*
@@ -1517,22 +1576,14 @@ connectDBStart(PGconn *conn)
goto connect_errReturn;
}
-#ifdef USE_SSL
- /* setup values based on SSL mode */
- if (conn->sslmode[0] == 'd') /* "disable" */
- conn->allow_ssl_try = false;
- else if (conn->sslmode[0] == 'a') /* "allow" */
- conn->wait_ssl_try = true;
-#endif
-
/*
- * Set up to try to connect, with protocol 3.0 as the first attempt.
+ * Set up to try to connect to the first address.
*/
conn->addrlist = addrs;
conn->addr_cur = addrs;
conn->addrlist_family = hint.ai_family;
- conn->pversion = PG_PROTOCOL(3, 0);
- conn->send_appname = true;
+ conn->try_next_addr = false;
+ conn->is_new_addr = true;
conn->status = CONNECTION_NEEDED;
/*
@@ -1546,6 +1597,12 @@ connectDBStart(PGconn *conn)
return 1;
connect_errReturn:
+
+ /*
+ * If we managed to open a socket, close it immediately rather than
+ * waiting till PQfinish. (The application cannot have gotten the socket
+ * from PQsocket yet, so this doesn't risk breaking anything.)
+ */
pqDropConnection(conn, true);
conn->status = CONNECTION_BAD;
return 0;
@@ -1607,6 +1664,7 @@ connectDBComplete(PGconn *conn)
case PGRES_POLLING_READING:
if (pqWaitTimed(1, 0, conn, finish_time))
{
+ /* hard failure, eg select() problem, aborts everything */
conn->status = CONNECTION_BAD;
return 0;
}
@@ -1615,6 +1673,7 @@ connectDBComplete(PGconn *conn)
case PGRES_POLLING_WRITING:
if (pqWaitTimed(0, 1, conn, finish_time))
{
+ /* hard failure, eg select() problem, aborts everything */
conn->status = CONNECTION_BAD;
return 0;
}
@@ -1663,6 +1722,7 @@ connectDBComplete(PGconn *conn)
PostgresPollingStatusType
PQconnectPoll(PGconn *conn)
{
+ bool need_new_connection = false;
PGresult *res;
char sebuf[256];
int optval;
@@ -1723,6 +1783,69 @@ PQconnectPoll(PGconn *conn)
keep_going: /* We will come back to here until there is
* nothing left to do. */
+
+ /* Time to advance to next address? */
+ if (conn->try_next_addr)
+ {
+ if (conn->addr_cur && conn->addr_cur->ai_next)
+ {
+ conn->addr_cur = conn->addr_cur->ai_next;
+ conn->is_new_addr = true;
+ }
+ else
+ {
+ /*
+ * Oops, no more addresses. An appropriate error message is
+ * already set up, so just set the right status.
+ */
+ goto error_return;
+ }
+ conn->try_next_addr = false;
+ }
+
+ /* Reset connection state machine? */
+ if (conn->is_new_addr)
+ {
+ /*
+ * (Re) initialize our connection control variables for a set of
+ * connection attempts to a single server address. These variables
+ * must persist across individual connection attempts, but we must
+ * reset them when we start to consider a new address (since it might
+ * not be the same server).
+ */
+ conn->pversion = PG_PROTOCOL(3, 0);
+ conn->send_appname = true;
+#ifdef USE_SSL
+ /* initialize these values based on SSL mode */
+ conn->allow_ssl_try = (conn->sslmode[0] != 'd'); /* "disable" */
+ conn->wait_ssl_try = (conn->sslmode[0] == 'a'); /* "allow" */
+#endif
+
+ conn->is_new_addr = false;
+ need_new_connection = true;
+ }
+
+ /* Force a new connection (perhaps to the same server as before)? */
+ if (need_new_connection)
+ {
+ /* Drop any existing connection */
+ pqDropConnection(conn, true);
+
+ /* Reset all state obtained from old server */
+ pqDropServerData(conn);
+
+ /* Drop any PGresult we might have, too */
+ conn->asyncStatus = PGASYNC_IDLE;
+ conn->xactStatus = PQTRANS_IDLE;
+ pqClearAsyncResult(conn);
+
+ /* Reset conn->status to put the state machine in the right state */
+ conn->status = CONNECTION_NEEDED;
+
+ need_new_connection = false;
+ }
+
+ /* Now try to advance the state machine for this connection */
switch (conn->status)
{
case CONNECTION_NEEDED:
@@ -1730,12 +1853,24 @@ 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
- * next one to try. We fail when we run out of addresses.
+ * next one to try.
+ *
+ * The extra level of braces here is historical. It's not
+ * worth reindenting this whole switch case to remove 'em.
*/
- while (conn->addr_cur != NULL)
{
struct addrinfo *addr_cur = conn->addr_cur;
+ if (addr_cur == NULL)
+ {
+ /*
+ * Ooops, no more addresses. An appropriate error
+ * message is already set up, so just set the right
+ * status.
+ */
+ goto error_return;
+ }
+
/* Remember current address for possible error msg */
memcpy(&conn->raddr.addr, addr_cur->ai_addr,
addr_cur->ai_addrlen);
@@ -1761,32 +1896,34 @@ keep_going: /* We will come back to here until there is
if (conn->sock == -1)
{
/*
- * ignore socket() failure if we have more addresses
- * to try
+ * Silently ignore socket() failure if we have more
+ * addresses to try; this reduces useless chatter in
+ * cases where the address list includes both IPv4 and
+ * IPv6 but kernel only accepts one family.
*/
if (addr_cur->ai_next != NULL)
{
- conn->addr_cur = addr_cur->ai_next;
- continue;
+ conn->try_next_addr = true;
+ goto keep_going;
}
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not create socket: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- break;
+ goto error_return;
}
/*
* Select socket options: no delay of outgoing data for
- * TCP sockets, nonblock mode, close-on-exec. Fail if any
- * of this fails.
+ * TCP sockets, nonblock mode, close-on-exec. Try the
+ * next address if any of this fails.
*/
if (!IS_AF_UNIX(addr_cur->ai_family))
{
if (!connectNoDelay(conn))
{
- pqDropConnection(conn, true);
- conn->addr_cur = addr_cur->ai_next;
- continue;
+ /* error message already created */
+ conn->try_next_addr = true;
+ goto keep_going;
}
}
if (!pg_set_noblock(conn->sock))
@@ -1794,9 +1931,8 @@ keep_going: /* We will come back to here until there is
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to non-blocking mode: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- pqDropConnection(conn, true);
- conn->addr_cur = addr_cur->ai_next;
- continue;
+ conn->try_next_addr = true;
+ goto keep_going;
}
#ifdef F_SETFD
@@ -1805,9 +1941,8 @@ keep_going: /* We will come back to here until there is
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not set socket to close-on-exec mode: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
- pqDropConnection(conn, true);
- conn->addr_cur = addr_cur->ai_next;
- continue;
+ conn->try_next_addr = true;
+ goto keep_going;
}
#endif /* F_SETFD */
@@ -1853,9 +1988,8 @@ keep_going: /* We will come back to here until there is
if (err)
{
- pqDropConnection(conn, true);
- conn->addr_cur = addr_cur->ai_next;
- continue;
+ conn->try_next_addr = true;
+ goto keep_going;
}
}
@@ -1934,25 +2068,13 @@ keep_going: /* We will come back to here until there is
}
/*
- * This connection failed --- set up error report, then
- * close socket (do it this way in case close() affects
- * the value of errno...). We will ignore the connect()
- * failure and keep going if there are more addresses.
+ * This connection failed. Add the error report to
+ * conn->errorMessage, then try the next address if any.
*/
connectFailureMessage(conn, SOCK_ERRNO);
- pqDropConnection(conn, true);
-
- /*
- * Try the next address, if any.
- */
- conn->addr_cur = addr_cur->ai_next;
- } /* loop over addresses */
-
- /*
- * Ooops, no more addresses. An appropriate error message is
- * already set up, so just set the right status.
- */
- goto error_return;
+ conn->try_next_addr = true;
+ goto keep_going;
+ }
}
case CONNECTION_STARTED:
@@ -1985,19 +2107,13 @@ keep_going: /* We will come back to here until there is
* error message.
*/
connectFailureMessage(conn, optval);
- pqDropConnection(conn, true);
/*
- * If more addresses remain, keep trying, just as in the
- * case where connect() returned failure immediately.
+ * Try the next address if any, just as in the case where
+ * connect() returned failure immediately.
*/
- if (conn->addr_cur->ai_next != NULL)
- {
- conn->addr_cur = conn->addr_cur->ai_next;
- conn->status = CONNECTION_NEEDED;
- goto keep_going;
- }
- goto error_return;
+ conn->try_next_addr = true;
+ goto keep_going;
}
/* Fill in the client address */
@@ -2266,12 +2382,13 @@ keep_going: /* We will come back to here until there is
{
/* only retry once */
conn->allow_ssl_try = false;
- /* Must drop the old connection */
- pqDropConnection(conn, true);
- conn->status = CONNECTION_NEEDED;
+ need_new_connection = true;
goto keep_going;
}
+ /* Else it's a hard failure */
+ goto error_return;
}
+ /* Else, return POLLING_READING or POLLING_WRITING status */
return pollres;
#else /* !USE_SSL */
/* can't get here */
@@ -2377,9 +2494,7 @@ keep_going: /* We will come back to here until there is
if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
{
conn->pversion = PG_PROTOCOL(2, 0);
- /* Must drop the old connection */
- pqDropConnection(conn, true);
- conn->status = CONNECTION_NEEDED;
+ need_new_connection = true;
goto keep_going;
}
@@ -2430,6 +2545,9 @@ keep_going: /* We will come back to here until there is
/* OK, we read the message; mark data consumed */
conn->inStart = conn->inCursor;
+ /* Check to see if we should mention pgpassfile */
+ dot_pg_pass_warning(conn);
+
#ifdef USE_SSL
/*
@@ -2443,9 +2561,7 @@ keep_going: /* We will come back to here until there is
{
/* only retry once */
conn->wait_ssl_try = false;
- /* Must drop the old connection */
- pqDropConnection(conn, true);
- conn->status = CONNECTION_NEEDED;
+ need_new_connection = true;
goto keep_going;
}
@@ -2454,14 +2570,13 @@ keep_going: /* We will come back to here until there is
* then do a non-SSL retry
*/
if (conn->sslmode[0] == 'p' /* "prefer" */
- && conn->allow_ssl_try
+ && conn->ssl != NULL
+ && conn->allow_ssl_try /* redundant? */
&& !conn->wait_ssl_try) /* redundant? */
{
/* only retry once */
conn->allow_ssl_try = false;
- /* Must drop the old connection */
- pqDropConnection(conn, true);
- conn->status = CONNECTION_NEEDED;
+ need_new_connection = true;
goto keep_going;
}
#endif
@@ -2620,9 +2735,7 @@ keep_going: /* We will come back to here until there is
{
PQclear(res);
conn->send_appname = false;
- /* Must drop the old connection */
- pqDropConnection(conn, true);
- conn->status = CONNECTION_NEEDED;
+ need_new_connection = true;
goto keep_going;
}
}
@@ -2702,8 +2815,6 @@ keep_going: /* We will come back to here until there is
error_return:
- dot_pg_pass_warning(conn);
-
/*
* We used to close the socket at this point, but that makes it awkward
* for those above us if they wish to remove this socket from their own
@@ -2830,13 +2941,7 @@ makeEmptyPGconn(void)
conn->std_strings = false; /* unless server says differently */
conn->verbosity = PQERRORS_DEFAULT;
conn->sock = -1;
- conn->auth_req_received = false;
- conn->password_needed = false;
conn->dot_pgpass_used = false;
-#ifdef USE_SSL
- conn->allow_ssl_try = true;
- conn->wait_ssl_try = false;
-#endif
/*
* We try to send at least 8K at a time, which is the usual size of pipe
@@ -2987,10 +3092,9 @@ freePGconn(PGconn *conn)
static void
closePGconn(PGconn *conn)
{
- PGnotify *notify;
- pgParameterStatus *pstatus;
-
/*
+ * If possible, send Terminate message to close the connection politely.
+ *
* Note that the protocol doesn't allow us to send Terminate messages
* during the startup phase.
*/
@@ -3020,32 +3124,15 @@ closePGconn(PGconn *conn)
conn->status = CONNECTION_BAD; /* Well, not really _bad_ - just
* absent */
conn->asyncStatus = PGASYNC_IDLE;
+ conn->xactStatus = PQTRANS_IDLE;
pqClearAsyncResult(conn); /* deallocate result */
resetPQExpBuffer(&conn->errorMessage);
pg_freeaddrinfo_all(conn->addrlist_family, conn->addrlist);
conn->addrlist = NULL;
conn->addr_cur = NULL;
- notify = conn->notifyHead;
- while (notify != NULL)
- {
- PGnotify *prev = notify;
- notify = notify->next;
- free(prev);
- }
- conn->notifyHead = conn->notifyTail = NULL;
- pstatus = conn->pstatus;
- while (pstatus != NULL)
- {
- pgParameterStatus *prev = pstatus;
-
- pstatus = pstatus->next;
- free(prev);
- }
- conn->pstatus = NULL;
- if (conn->lobjfuncs)
- free(conn->lobjfuncs);
- conn->lobjfuncs = NULL;
+ /* Reset all state obtained from server, too */
+ pqDropServerData(conn);
}
/*
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 8843ccb..eaa80b6 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -375,6 +375,8 @@ struct pg_conn
bool sigpipe_flag; /* can we mask SIGPIPE via MSG_NOSIGNAL? */
/* Transient state needed while establishing connection */
+ bool try_next_addr; /* time to advance to next address? */
+ bool is_new_addr; /* need to (re)initialize for new address? */
struct addrinfo *addrlist; /* list of possible backend addresses */
struct addrinfo *addr_cur; /* the one currently being tried */
int addrlist_family; /* needed to know how to free addrlist */
--
2.17.1