Fixing pool_read2

Hi,

Recently Emond Papegaaij reported me off list that pool_read2() has a
bug and sent a patch (see below).

I agree that pool_read2() should check whether "len" argument has a
positive value. However, if pool_read2() reurns NULL when len == 0 (as
the patch instructs), quite a few regression test fail. This is
because in many places callers of pool_read2() expect it returns non
NULL if len == 0. Otherwise, they complain:

ERROR: unable to forward message to frontend
DETAIL: read from backend failed

pool_read2() is typically used to read the packet body (which is after
the packet lenth field). Even if len == 0 case (no body packet like
BindComplete), callers feel free to call palloc with 0 byte, and
memcpy with 0 byte. Fortunately palloc and memcpy are flexible enough
to accept 0 byte argument. Maybe we should teach the callers to check
len == 0 or not, but there are too many such places. For now, I would
like to keep the current behavior (allow to length == 0). Attached is
a patch to implement that way. (with slight commit message change and
indentation change in the code to follow our coding standard).

-----------------------------------------------------------------------
From 83278ae402affafdc0ff0278a76c13476345b71f Mon Sep 17 00:00:00 2001
From: Emond Papegaaij <emond(dot)papegaaij(at)topicus(dot)nl>
Date: Fri, 24 Apr 2026 23:40:34 +0200
Subject: [PATCH] Reject non-positive length in pool_read2.

pool_read2() in src/utils/pool_stream.c allocates buf2 sized to
cp->len + len, consumes that many bytes from the pending buffer,
then loops reading from the socket while len > 0. When len is zero
or negative the loop never executes a fresh read and the function
falls straight through to "return cp->buf2", handing the caller a
pointer to whatever the previous pool_read2() invocation happened
to leave in the per-connection buffer.

Callers compute len from on-the-wire length fields, e.g.
len = ntohl(hdr) - sizeof(hdr);
and several do so without bounds-checking. A backend that ships a
malformed message with a length smaller than the header (or a
negative value once cast to int32) drives pool_read2() into the
stale-buffer path. The stale bytes are then forwarded to the
frontend as ParameterStatus / ErrorResponse / etc. payloads,
leaking whatever the previous reply contained on this same
backend connection.

Add a guard at the top of pool_read2() that logs and returns NULL
when len <= 0, before any buffer setup runs. The function's
existing comment already claims a NULL return for the failure
case; this aligns the implementation with that contract for the
non-positive-len input. A separate audit of the ~40 call sites,
to make every caller treat a NULL return consistently rather than
dereference it, is deferred to a follow-up consolidation patch:
this change shuts the immediate stale-data leak without fanning
out call-site churn across release branches.
---
src/utils/pool_stream.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/utils/pool_stream.c b/src/utils/pool_stream.c
index 0a9cc3576..9bee94b3c 100644
--- a/src/utils/pool_stream.c
+++ b/src/utils/pool_stream.c
@@ -305,6 +305,8 @@ pool_read(POOL_CONNECTION *cp, void *buf, int len)
/*
* read exactly len bytes from cp
* returns buffer address on success otherwise NULL.
+* Returns NULL when len <= 0; callers must already tolerate a NULL
+* return because the read-error paths below also return NULL.
*/
char *
pool_read2(POOL_CONNECTION *cp, int len)
@@ -314,7 +316,16 @@ pool_read2(POOL_CONNECTION *cp, int len)
int alloc_size;
int consume_size;
int readlen;
- MemoryContext oldContext = SwitchToConnectionContext(cp->isbackend);
+ MemoryContext oldContext;
+
+ if (len <= 0)
+ {
+ ereport(LOG,
+ (errmsg("pool_read2: non-positive len %d, returning NULL", len)));
+ return NULL;
+ }
+
+ oldContext = SwitchToConnectionContext(cp->isbackend);

req_size = cp->len + len;

--
2.51.0
-----------------------------------------------------------------------

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp