public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Peter Eisentraut <[email protected]>
To: solai v <[email protected]>
To: Jacob Champion <[email protected]>
Cc: Mark Dilger <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Lars Kanis <[email protected]>
Cc: [email protected]
Cc: Merlin Moncure <[email protected]>
Subject: Re: libpq: Process buffered SSL read bytes to support records >8kB on async API
Date: Thu, 9 Jul 2026 15:55:59 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAOYmi+=jZNU9mZ4Z83aR0RiqZ2pF35u5040gCxBgcFqoA4oNbQ@mail.gmail.com>
<CAOYmi+mpymrgZ76Jre2dx_PwRniS9YZojwH0rZnTuiGHCsj0rA@mail.gmail.com>
<CAOYmi+=kbb7pV7k7RfWUcFt=CArJmp1R+R1Xnb8UnZLwpFgOwA@mail.gmail.com>
<CAOYmi+==Y5ZZiwukN-wzGCyxdrL1dU3wOG-j0nza3MC3R2XhcA@mail.gmail.com>
<kl6inl3iyycxqpiedmnnzaz2ytid5dw6knwwqk5plg6ej34oi6@fbyu4ll7ebzn>
<CAOYmi+kZhv_LCg=g8Z6xtiueSYdz1fCvrXwAJ-i9m3VMm9_tXw@mail.gmail.com>
<CAOYmi+mD6EbDEYfwZON0FCUAvGO+2=jR2V4KQYx+d+g0ap0Amg@mail.gmail.com>
<CAOYmi+=YJ2J_sQ6yS8y1o9AX1Xgz35_r0hApV8QRWwi1K8gC0g@mail.gmail.com>
<[email protected]>
<CAHgHdKseseJG0j_96xrOuP3LgubvGScvXsQyPCYWANnBxTL4tA@mail.gmail.com>
<[email protected]>
<CAF0whuf=-G3SwXSTWyZNO148qoaEmth71cM7XBuX4X+2PnQkPQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
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
Attachments:
[text/x-patch] 0001-libpq-Make-error-checks-in-the-new-buffer-draining-c.patch (2.6K, ../[email protected]/2-0001-libpq-Make-error-checks-in-the-new-buffer-draining-c.patch)
download | inline diff:
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
view thread (10+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: libpq: Process buffered SSL read bytes to support records >8kB on async API
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox