From: Tom Lane <tgl@sss.pgh.pa.us>
To: lars@greiz-reinsdorf.de
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18210: libpq: PQputCopyData sometimes fails in non-blocking mode over GSSAPI encrypted connection
Date: Wed, 22 Nov 2023 17:29:34 -0500
Message-ID: <2524668.1700692174@sss.pgh.pa.us> (raw)
In-Reply-To: <2199742.1700680746@sss.pgh.pa.us>
References: <18210-4c6d0b14627f2eb8@postgresql.org>
<1675678.1700665886@sss.pgh.pa.us>
<2199742.1700680746@sss.pgh.pa.us>
I wrote:
> I wonder if we should drop the idea of returning a positive bytecount
> after a partial write, and just return the pqsecure_raw_write result,
> and not reset PqGSSSendConsumed until we write everything presented.
> In edge cases maybe that would result in some buffer bloat, but it
> doesn't seem worse than what happens when the very first
> pqsecure_raw_write returns EINTR.
Here's a patch that fixes it along those lines. I like this better,
I think, not least because it removes the assumption that "interesting"
pqsecure_raw_write failures will recur on the next try.
Still need to look at syncing the backend with this.
regards, tom lane
Attachments:
[text/x-diff] 0003-alternative-gssapi-fix.patch (5.1K, ../2524668.1700692174@sss.pgh.pa.us/2-0003-alternative-gssapi-fix.patch)
download | inline diff:diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.cindex 7e373236e9..ea8b0020d2 100644--- a/src/interfaces/libpq/fe-secure-gssapi.c+++ b/src/interfaces/libpq/fe-secure-gssapi.c@@ -79,8 +79,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)
@@ -90,19 +90,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)
{
@@ -140,33 +146,20 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
retval = pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, amount);
if (retval <= 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 retval;
- }
/*
* Check if this was a partial write, and if so, move forward that
* far in our buffer and try again.
*/
- if (retval != amount)+ if (retval < amount)
{
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;
}
/*
@@ -192,7 +185,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);
@@ -236,10 +229,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.hindex c745facfec..22bc682ffc 100644--- a/src/interfaces/libpq/libpq-int.h+++ b/src/interfaces/libpq/libpq-int.h@@ -583,8 +583,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 */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, lars@greiz-reinsdorf.de, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18210: libpq: PQputCopyData sometimes fails in non-blocking mode over GSSAPI encrypted connection
In-Reply-To: <2524668.1700692174@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox