On 22/06/2026 10:56, Peter Eisentraut wrote:
> I found a couple of places where a pgoff_t value, typically a signed 64-
> bit integer, is cast to a type with a smaller range. I noticed this
> specifically in error messages, so it's mostly cosmetic. Also, files
> with a size where this would matter are not expected in many places, but
> I suspect that the case in basebackup_server.c in the attached patch can
> actually happen.
>
> The fix is to use the %lld print format and a cast to long long int,
> which is the style already in most places.
>
> In passing, I also converted a few places that used %llu to print
> pgoff_t to also use %lld instead. This is mostly for consistency, but
> it also feels more robust in case a negative value ever sneaks in.
+1
In SendTimeLineHistory():
> /* XXX might truncate histfilelen */
> Assert(histfilelen <= UINT32_MAX);
> pq_sendint32(&buf, histfilelen); /* col2 len */
>
> bytesleft = histfilelen;
> while (bytesleft > 0)
> {
> PGAlignedBlock rbuf;
> int nread;
>
> pgstat_report_wait_start(WAIT_EVENT_WALSENDER_TIMELINE_HISTORY_READ);
> nread = read(fd, rbuf.data, sizeof(rbuf));
> pgstat_report_wait_end();
> if (nread < 0)
> ereport(ERROR,
> (errcode_for_file_access(),
> errmsg("could not read file \"%s\": %m",
> path)));
> else if (nread == 0)
> ereport(ERROR,
> (errcode(ERRCODE_DATA_CORRUPTED),
> errmsg("could not read file \"%s\": read %d of %zu",
> path, nread, (Size) bytesleft)));
>
> pq_sendbytes(&buf, rbuf.data, nread);
> bytesleft -= nread;
> }
Not that it makes much difference, but I'd suggest "if (histfilelen >
UINT32_MAX) elog(ERROR, ...)" here instead of an Assert. This isn't
performance critical and a better error message is always nice if
something weird happens. (I think on non-assertion builds, you'd get
"out of memory" error while trying to increase the send buffer).
Not new with this patch, but I noticed that if the file increases in
size while we're reading it for some reason, we would read beyond the
originally calculated length. It really shouldn't happen, but it'd be
good to add an "nread <= bytesleft" check here, for the sake of robustness.
- Heikki
