Re: [PATCH] Fix two client-side NULL dereferences on malformed result sets (libpq, pg_receivewal)

> On Nov 9, 2026, at 01:59, s1ko <s1ko(at)riseup(dot)net> wrote:
>
> Hi,
>
> Two client programs crash when the server answers an internal query with a
> result set of an unexpected shape. Both are plain NULL dereferences: the code
> checks part of the shape, then calls PQgetvalue() on a cell that does not
> exist. (First sent to security@, Noah pointed me here since the project does
> not treat these as security issues.)
>
> 0001 - libpq, fe-connect.c. PQconnectPoll gates the target_session_attrs
> probes ("SHOW transaction_read_only" and "SELECT pg_is_in_recovery()") on
> PQntuples(res) == 1 but never on PQnfields(res), then does
> strncmp(PQgetvalue(res, 0, 0), ...). One row and zero columns gives
> strncmp(NULL, ...). The patch requires PQnfields(res) == 1 too, so a bad reply
> takes the existing "Something went wrong" error path and the connection
> attempt fails.
>
> 0002 - pg_basebackup, receivelog.c. The TIMELINE_HISTORY shape check logs a
> warning and falls through to writeTimeLineHistoryFile(PQgetvalue(res, 0, 0),
> PQgetvalue(res, 0, 1)), which calls strlen() on the NULL right away. The patch
> makes the check fatal (pg_log_error, PQclear, return false), matching the
> other error returns in that block. It has been like this since cc8d4151172.
>
> Tested on master at 0607aa6109 with clang 21 and -fsanitize=address,undefined
> against a small fake server on loopback: both crash before the patch, both
> report a clean error after it, and the well-formed control paths still work
> (libpq connects, pg_receivewal writes the history file and moves on to
> START_REPLICATION). Both fixes apply cleanly to the back branches as far as
> I checked, but I have only run them on master.
>
> Regards,
> s1ko<v1-0001-libpq-check-column-count-before-reading-target_se.patch><v1-0002-pg_basebackup-fail-on-malformed-TIMELINE_HISTORY-.patch>

For 0001, the change is in libpq. I think we should accept the patch to make libpq more robust, for the following reasons:

1) PG allows a query to return zero columns, for example:
```
evantest=# select;
--
(1 row)
```

2) libpq can communicate with different PG protocol implementations, such as a normal PG server, a PG-compatible database server, or a pooler like PgBouncer. A buggy or malicious implementation could return an unexpected result.

3) With 0001, if either of the two internal queries returns zero columns, execution falls through to the existing error-handling path, which calls libpq_append_conn_error(), instead of crashing.

So 0001 looks good to me.

For 0002, since the existing code already checks if PQnfields(res) != 2, we should definitely not proceed to call writeTimeLineHistoryFile() when the response has an unexpected number of columns, as that function needs to read the first two columns.

So 0002 also looks good to me.

One related issue I noticed while reviewing 0002 is that writeTimeLineHistoryFile() returns a bool result for success or failure, but ReceiveXlogStream() ignores the return value. This might be outside the scope of this patch. I'll investigate it further and may start a separate thread to discuss the issue.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/