summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 50d3f09)
raw | patch | inline | side by side (parent: 50d3f09)
| author | Tom Lane <tgl@sss.pgh.pa.us> | |
| Thu, 23 Nov 2023 18:30:18 +0000 (13:30 -0500) | ||
| committer | Tom Lane <tgl@sss.pgh.pa.us> | |
| Thu, 23 Nov 2023 18:30:18 +0000 (13:30 -0500) |
When using GSSAPI encryption in non-blocking mode, libpq sometimes
failed with "GSSAPI caller failed to retransmit all data needing
to be retried". The cause is that pqPutMsgEnd rounds its transmit
request down to an even multiple of 8K, and sometimes that can lead
to not requesting a write of data that was requested to be written
(but reported as not written) earlier. That can upset pg_GSS_write's
logic for dealing with not-yet-written data, since it's possible
the data in question had already been incorporated into an encrypted
packet that we weren't able to send during the previous call.
We could fix this with a one-or-two-line hack to disable pqPutMsgEnd's
round-down behavior, but that seems like making the caller work around
a behavior that pg_GSS_write shouldn't expose in this way. Instead,
adjust pg_GSS_write to never report a partial write: it either
reports a complete write, or reflects the failure of the lower-level
pqsecure_raw_write call. The requirement still exists for the caller
to present at least as much data as on the previous call, but with
the caller-visible write start point not moving there is no temptation
for it to present less. We lose some ability to reclaim buffer space
early, but I doubt that that will make much difference in practice.
This also gets rid of a rather dubious assumption that "any
interesting failure condition (from pqsecure_raw_write) will recur
on the next try". We've not seen failure reports traceable to that,
but I've never trusted it particularly and am glad to remove it.
Make the same adjustments to the equivalent backend routine
be_gssapi_write(). It is probable that there's no bug on the backend
side, since we don't have a notion of nonblock mode there; but we
should keep the logic the same to ease future maintenance.
Per bug #18210 from Lars Kanis. Back-patch to all supported branches.
Discussion: https://postgr.es/m/18210-4c6d0b14627f2eb8@postgresql.org
failed with "GSSAPI caller failed to retransmit all data needing
to be retried". The cause is that pqPutMsgEnd rounds its transmit
request down to an even multiple of 8K, and sometimes that can lead
to not requesting a write of data that was requested to be written
(but reported as not written) earlier. That can upset pg_GSS_write's
logic for dealing with not-yet-written data, since it's possible
the data in question had already been incorporated into an encrypted
packet that we weren't able to send during the previous call.
We could fix this with a one-or-two-line hack to disable pqPutMsgEnd's
round-down behavior, but that seems like making the caller work around
a behavior that pg_GSS_write shouldn't expose in this way. Instead,
adjust pg_GSS_write to never report a partial write: it either
reports a complete write, or reflects the failure of the lower-level
pqsecure_raw_write call. The requirement still exists for the caller
to present at least as much data as on the previous call, but with
the caller-visible write start point not moving there is no temptation
for it to present less. We lose some ability to reclaim buffer space
early, but I doubt that that will make much difference in practice.
This also gets rid of a rather dubious assumption that "any
interesting failure condition (from pqsecure_raw_write) will recur
on the next try". We've not seen failure reports traceable to that,
but I've never trusted it particularly and am glad to remove it.
Make the same adjustments to the equivalent backend routine
be_gssapi_write(). It is probable that there's no bug on the backend
side, since we don't have a notion of nonblock mode there; but we
should keep the logic the same to ease future maintenance.
Per bug #18210 from Lars Kanis. Back-patch to all supported branches.
Discussion: https://postgr.es/m/18210-4c6d0b14627f2eb8@postgresql.org
index 56d310e61a27e4db3b81531f42538a0b9f334cbc..0fc7d565e2a92fc3540b3c8ad83243f534da14ba 100644 (file)
static int PqGSSSendLength; /* End of data available in PqGSSSendBuffer */
static int PqGSSSendNext; /* Next index to send a byte from
* PqGSSSendBuffer */
-static int PqGSSSendConsumed; /* Number of *unencrypted* bytes consumed for
- * current contents of PqGSSSendBuffer */
+static int PqGSSSendConsumed; /* Number of source bytes encrypted but not
+ * yet reported as sent */
static char *PqGSSRecvBuffer; /* Received, encrypted data */
static int PqGSSRecvLength; /* End of data available in PqGSSRecvBuffer */
*
* On success, returns the number of data bytes consumed (possibly less than
* len). On failure, returns -1 with errno set appropriately. For retryable
- * errors, caller should call again (passing the same data) once the socket
- * is ready.
+ * errors, caller should call again (passing the same or more data) once the
+ * socket is ready.
*
* Dealing with fatal errors here is a bit tricky: we can't invoke elog(FATAL)
* since it would try to write to the client, probably resulting in infinite
minor;
gss_buffer_desc input,
output;
- size_t bytes_sent = 0;
size_t bytes_to_encrypt;
size_t bytes_encrypted;
gss_ctx_id_t gctx = port->gss->ctx;
/*
- * When we get a failure, we must not tell the caller we have successfully
- * transmitted everything, else it won't retry. Hence a "success"
- * (positive) return value must only count source bytes corresponding to
- * fully-transmitted encrypted packets. The amount of source data
- * corresponding to the current partly-transmitted packet is remembered in
+ * When we get a retryable failure, we must not tell the caller we have
+ * successfully transmitted everything, else it won't retry. For
+ * simplicity, we claim we haven't transmitted anything until we have
+ * successfully transmitted all "len" bytes. Between calls, the amount of
+ * the current input data that's already been encrypted and placed into
+ * PqGSSSendBuffer (and perhaps transmitted) is remembered in
* PqGSSSendConsumed. On a retry, the caller *must* be sending that data
* again, so if it offers a len less than that, something is wrong.
+ *
+ * Note: it may seem attractive to report partial write completion once
+ * we've successfully sent any encrypted packets. However, that can cause
+ * problems for callers; notably, pqPutMsgEnd's heuristic to send only
+ * full 8K blocks interacts badly with such a hack. We won't save much,
+ * typically, by letting callers discard data early, so don't risk it.
*/
if (len < PqGSSSendConsumed)
{
errno = ECONNRESET;
return -1;
}
+
/* Discount whatever source data we already encrypted. */
bytes_to_encrypt = len - PqGSSSendConsumed;
bytes_encrypted = PqGSSSendConsumed;
ret = secure_raw_write(port, PqGSSSendBuffer + PqGSSSendNext, amount);
if (ret <= 0)
- {
- /*
- * Report any previously-sent data; if there was none, reflect
- * the secure_raw_write result up to our caller. When there
- * was some, we're effectively assuming that any interesting
- * failure condition will recur on the next try.
- */
- if (bytes_sent)
- return bytes_sent;
return ret;
- }
/*
* Check if this was a partial write, and if so, move forward that
* far in our buffer and try again.
*/
- if (ret != amount)
+ if (ret < amount)
{
PqGSSSendNext += ret;
continue;
}
- /* We've successfully sent whatever data was in that packet. */
- bytes_sent += PqGSSSendConsumed;
-
- /* All encrypted data was sent, our buffer is empty now. */
- PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
+ /* We've successfully sent whatever data was in the buffer. */
+ PqGSSSendLength = PqGSSSendNext = 0;
}
/*
output.value = NULL;
output.length = 0;
- /* Create the next encrypted packet */
+ /*
+ * Create the next encrypted packet. Any failure here is considered a
+ * hard failure, so we return -1 even if some data has been sent.
+ */
major = gss_wrap(&minor, gctx, 1, GSS_C_QOP_DEFAULT,
&input, &conf_state, &output);
if (major != GSS_S_COMPLETE)
}
/* If we get here, our counters should all match up. */
- Assert(bytes_sent == len);
- Assert(bytes_sent == bytes_encrypted);
+ Assert(len == PqGSSSendConsumed);
+ Assert(len == bytes_encrypted);
+
+ /* We're reporting all the data as sent, so reset PqGSSSendConsumed. */
+ PqGSSSendConsumed = 0;
- return bytes_sent;
+ return bytes_encrypted;
}
/*
index f37fce4f08ec67e21c49a3a8725c1a85e968170f..ac2a8c8563f3c551f5d56855e7724e785120b4e8 100644 (file)
* On success, returns the number of data bytes consumed (possibly less than
* len). On failure, returns -1 with errno set appropriately. If the errno
* indicates a non-retryable error, a message is put into conn->errorMessage.
- * For retryable errors, caller should call again (passing the same data)
- * once the socket is ready.
+ * For retryable errors, caller should call again (passing the same or more
+ * data) once the socket is ready.
*/
ssize_t
pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
gss_buffer_desc input,
output = GSS_C_EMPTY_BUFFER;
ssize_t ret = -1;
- size_t bytes_sent = 0;
size_t bytes_to_encrypt;
size_t bytes_encrypted;
gss_ctx_id_t gctx = conn->gctx;
/*
- * When we get a failure, we must not tell the caller we have successfully
- * transmitted everything, else it won't retry. Hence a "success"
- * (positive) return value must only count source bytes corresponding to
- * fully-transmitted encrypted packets. The amount of source data
- * corresponding to the current partly-transmitted packet is remembered in
+ * When we get a retryable failure, we must not tell the caller we have
+ * successfully transmitted everything, else it won't retry. For
+ * simplicity, we claim we haven't transmitted anything until we have
+ * successfully transmitted all "len" bytes. Between calls, the amount of
+ * the current input data that's already been encrypted and placed into
+ * PqGSSSendBuffer (and perhaps transmitted) is remembered in
* PqGSSSendConsumed. On a retry, the caller *must* be sending that data
* again, so if it offers a len less than that, something is wrong.
+ *
+ * Note: it may seem attractive to report partial write completion once
+ * we've successfully sent any encrypted packets. However, that can cause
+ * problems for callers; notably, pqPutMsgEnd's heuristic to send only
+ * full 8K blocks interacts badly with such a hack. We won't save much,
+ * typically, by letting callers discard data early, so don't risk it.
*/
if (len < PqGSSSendConsumed)
{
*/
if (PqGSSSendLength)
{
- ssize_t ret;
+ ssize_t retval;
ssize_t amount = PqGSSSendLength - PqGSSSendNext;
- ret = pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, amount);
- if (ret <= 0)
- {
- /*
- * Report any previously-sent data; if there was none, reflect
- * the pqsecure_raw_write result up to our caller. When there
- * was some, we're effectively assuming that any interesting
- * failure condition will recur on the next try.
- */
- if (bytes_sent)
- return bytes_sent;
- return ret;
- }
+ retval = pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, amount);
+ if (retval <= 0)
+ return retval;
/*
* Check if this was a partial write, and if so, move forward that
* far in our buffer and try again.
*/
- if (ret != amount)
+ if (retval < amount)
{
- PqGSSSendNext += ret;
+ PqGSSSendNext += retval;
continue;
}
- /* We've successfully sent whatever data was in that packet. */
- bytes_sent += PqGSSSendConsumed;
-
- /* All encrypted data was sent, our buffer is empty now. */
- PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
+ /* We've successfully sent whatever data was in the buffer. */
+ PqGSSSendLength = PqGSSSendNext = 0;
}
/*
/*
* Create the next encrypted packet. Any failure here is considered a
- * hard failure, so we return -1 even if bytes_sent > 0.
+ * hard failure, so we return -1 even if some data has been sent.
*/
major = gss_wrap(&minor, gctx, 1, GSS_C_QOP_DEFAULT,
&input, &conf_state, &output);
}
/* If we get here, our counters should all match up. */
- Assert(bytes_sent == len);
- Assert(bytes_sent == bytes_encrypted);
+ Assert(len == PqGSSSendConsumed);
+ Assert(len == bytes_encrypted);
+
+ /* We're reporting all the data as sent, so reset PqGSSSendConsumed. */
+ PqGSSSendConsumed = 0;
- ret = bytes_sent;
+ ret = bytes_encrypted;
cleanup:
/* Release GSSAPI buffer storage, if we didn't already */
index efce6c6afd771e92a973914a19079c4f775da0cc..668a5c874982c2997cd35de4d9e99efd418e495f 100644 (file)
int gss_SendLength; /* End of data available in gss_SendBuffer */
int gss_SendNext; /* Next index to send a byte from
* gss_SendBuffer */
- int gss_SendConsumed; /* Number of *unencrypted* bytes consumed
- * for current contents of gss_SendBuffer */
+ int gss_SendConsumed; /* Number of source bytes encrypted but
+ * not yet reported as sent */
char *gss_RecvBuffer; /* Received, encrypted data */
int gss_RecvLength; /* End of data available in gss_RecvBuffer */
char *gss_ResultBuffer; /* Decryption of data in gss_RecvBuffer */