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
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>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 22 Nov 2023 14:19:06 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2518742.1700692142.0@sss.pgh.pa.us>
Date: Wed, 22 Nov 2023 17:29:34 -0500
Message-ID: <2524668.1700692174@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2524668.1700692174%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2518742.1700692142.1@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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0003-alternative-gssapi-fix.patch";
	charset="us-ascii"
Content-ID: <2518742.1700692142.2@sss.pgh.pa.us>
Content-Description: 0003-alternative-gssapi-fix.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libp=
q/fe-secure-gssapi.c
index 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 t=
han
  * len).  On failure, returns -1 with errno set appropriately.  If the er=
rno
  * indicates a non-retryable error, a message is added to conn->errorMess=
age.
- * 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 mo=
re
+ * 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 =3D GSS_C_EMPTY_BUFFER;
 	ssize_t		ret =3D -1;
-	size_t		bytes_sent =3D 0;
 	size_t		bytes_to_encrypt;
 	size_t		bytes_encrypted;
 	gss_ctx_id_t gctx =3D conn->gctx;
 =

 	/*
-	 * When we get a failure, we must not tell the caller we have successful=
ly
-	 * 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 dat=
a
 	 * 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 cau=
se
+	 * 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 l=
en)
 =

 			retval =3D pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, a=
mount);
 			if (retval <=3D 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 !=3D amount)
+			if (retval < amount)
 			{
 				PqGSSSendNext +=3D retval;
 				continue;
 			}
 =

-			/* We've successfully sent whatever data was in that packet. */
-			bytes_sent +=3D PqGSSSendConsumed;
-
-			/* All encrypted data was sent, our buffer is empty now. */
-			PqGSSSendLength =3D PqGSSSendNext =3D PqGSSSendConsumed =3D 0;
+			/* We've successfully sent whatever data was in the buffer. */
+			PqGSSSendLength =3D PqGSSSendNext =3D 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 =3D 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 l=
en)
 	}
 =

 	/* If we get here, our counters should all match up. */
-	Assert(bytes_sent =3D=3D len);
-	Assert(bytes_sent =3D=3D bytes_encrypted);
+	Assert(len =3D=3D PqGSSSendConsumed);
+	Assert(len =3D=3D bytes_encrypted);
+
+	/* We're reporting all the data as sent, so reset PqGSSSendConsumed. */
+	PqGSSSendConsumed =3D 0;
 =

-	ret =3D bytes_sent;
+	ret =3D 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 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 */

------- =_aaaaaaaaaa0--