On 08/07/2026 22:16, Peter Eisentraut wrote:
In pqDrainPending(), there is
nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd,
bytes_pending);
conn->inEnd += nread;
But pqsecure_read() can return -1 for error. So adding that to conn-
>inEnd at that point seems wrong.
There is error handling in the following code, but this would still kind
of corrupt the conn->inEnd value?
/* When there are bytes pending, the read function is not supposed
to fail */
if (nread != bytes_pending)
{
libpq_append_conn_error(conn,
"drained only %zu of %zd pending bytes
in transport buffer",
nread, bytes_pending);
return -1;
}
I'm not sure if that comment means that a -1 return cannot happen? Or
just that the whole error check should not happen?
The comment in pgsecure_bytes_pending() says:
* If pqsecure_read() is called for this number of bytes, it's guaranteed to
* return successfully without reading from the underlying socket. See
* pqDrainPending() for a more complete discussion of the concepts involved.
so as long as pqsecure_read() and pqsecure_read_pending() honor that
contract, pqsecure_read() should not return an error. I agree that's a
bit sloppy though, we should still check the return value.
The intention was also that it cannot do a short read, but that wasn't
quite clear from the comment either.
Also note that the format placeholder for nread is wrong. If you do get
a -1, it will print some large unsigned value.
Good catch.
Patch attached to clean up all that.
- Heikki
From be68e94f81d8bb830cfda6eee6a20a41ec335933 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Thu, 9 Jul 2026 15:51:30 +0300
Subject: [PATCH 1/1] libpq: Make error checks in the new buffer draining code
more robust
Check explicitly for pqsecure_read() returning an error. It shouldn't
fail, and we would've caught it in the check for a short read, but
better to be explicit so that the error message is more informative.
We also shouldn't update 'inEnd' when the read fails, although that
too is just pro forma as we will bail out and close the connection on
error.
Reported-by: Peter Eisentraut <[email protected]>
Discussion: https://www.postgresql.org/message-id/[email protected]
---
src/interfaces/libpq/fe-misc.c | 11 ++++++++---
src/interfaces/libpq/fe-secure.c | 5 +++--
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index dd772838005..f11b58bf9c7 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -928,13 +928,18 @@ pqDrainPending(PGconn *conn)
nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd,
bytes_pending);
- conn->inEnd += nread;
- /* When there are bytes pending, the read function is not supposed to fail */
+ /*
+ * When there are bytes pending, pqsecure_read() is not supposed to fail
+ * or do a short read, but let's check anyway to be safe.
+ */
+ if (nread < 0)
+ return -1;
+ conn->inEnd += nread;
if (nread != bytes_pending)
{
libpq_append_conn_error(conn,
- "drained only %zu of %zd pending bytes in transport buffer",
+ "drained only %zd of %zd pending bytes in transport buffer",
nread, bytes_pending);
return -1;
}
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 70faf8b2fe0..1a8e2e6746e 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -247,8 +247,9 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
* Return the number of bytes available in the transport buffer.
*
* If pqsecure_read() is called for this number of bytes, it's guaranteed to
- * return successfully without reading from the underlying socket. See
- * pqDrainPending() for a more complete discussion of the concepts involved.
+ * return successfully with the same number of bytes, without reading from the
+ * underlying socket. See pqDrainPending() for a more complete discussion of
+ * the concepts involved.
*/
ssize_t
pqsecure_bytes_pending(PGconn *conn)
--
2.47.3
