This is an automated email from the ASF dual-hosted git repository. leborchuk pushed a commit to branch REL_2_STABLE in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit e45a60bb0f7b1f0866dc41a529ae023d35db6cc2 Author: Tom Lane <[email protected]> AuthorDate: Thu Nov 23 13:30:18 2023 -0500 Fix timing-dependent failure in GSSAPI data transmission. 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/[email protected] (cherry picked from commit 8f23e6a45009424ed6e01d95d066955e2b38cfc4) --- src/backend/libpq/be-secure-gssapi.c | 60 +++++++++++++++---------------- src/interfaces/libpq/fe-secure-gssapi.c | 62 +++++++++++++++------------------ src/interfaces/libpq/libpq-int.h | 4 +-- 3 files changed, 61 insertions(+), 65 deletions(-) diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c index 19b97fbbce2..8ea04645201 100644 --- a/src/backend/libpq/be-secure-gssapi.c +++ b/src/backend/libpq/be-secure-gssapi.c @@ -60,8 +60,8 @@ static char *PqGSSSendBuffer; /* Encrypted data waiting to be sent */ 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 */ @@ -83,8 +83,8 @@ static uint32 PqGSSMaxPktSize; /* Maximum size we can encrypt and fit the * * 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 @@ -98,19 +98,25 @@ be_gssapi_write(Port *port, void *ptr, size_t len) 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) { @@ -118,6 +124,7 @@ be_gssapi_write(Port *port, void *ptr, size_t len) errno = ECONNRESET; return -1; } + /* Discount whatever source data we already encrypted. */ bytes_to_encrypt = len - PqGSSSendConsumed; bytes_encrypted = PqGSSSendConsumed; @@ -146,33 +153,20 @@ be_gssapi_write(Port *port, void *ptr, size_t len) 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; } /* @@ -196,7 +190,10 @@ be_gssapi_write(Port *port, void *ptr, size_t len) 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) @@ -240,10 +237,13 @@ be_gssapi_write(Port *port, void *ptr, size_t len) } /* 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; } /* diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c index aeb6e35dbdd..fdd9878a01f 100644 --- a/src/interfaces/libpq/fe-secure-gssapi.c +++ b/src/interfaces/libpq/fe-secure-gssapi.c @@ -86,8 +86,8 @@ * 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 added to 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) @@ -97,19 +97,25 @@ 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) { @@ -142,38 +148,25 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len) */ 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; } /* @@ -199,7 +192,7 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len) /* * 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); @@ -245,10 +238,13 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len) } /* 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 */ diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 7a7a3921e27..e03941b755e 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -608,8 +608,8 @@ struct pg_conn 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 */ --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
