Re: BUG #19598: pg_waldump: -s/-e accept out-of-range WAL locations and silently use the low 32 bits

On Fri, Aug 14, 2026 at 3:33 PM Zexin Li <lizi(dot)openmind(at)gmail(dot)com> wrote:
>
> On Wed, Aug 12, 2026, Fujii Masao wrote:
> > I've pushed the patch. Thanks!
>
> Thank you for committing this!
>
> Attached is the separate patch you suggested for the remaining
> frontend sscanf("%X/%08X") call sites: the LSNs that pg_basebackup
> and pg_rewind read from server responses, pg_rewind reads from
> timeline history files, and pg_combinebackup reads from backup_label
> files.

Thanks for the patch!

> The backend's copies of these parsers are left untouched,

Okay, the backend sscanf()-based LSN parsers can be handled separately
in a later patch.

> and parse_manifest.c is already being handled by Ayush's patch.

Okay.

> * Two of the converted call sites read a location out of a longer
> line, so the patch adds pg_parse_lsn_prefix(), which reports the
> first character after the location instead of requiring the string
> to end there, and reimplements pg_parse_lsn() on top of it, keeping
> a single implementation of the syntax rules. Each tool keeps its
> existing error message.

I wonder if we really need pg_parse_lsn_prefix() for this. Instead,
how about isolating the LSN token by temporarily NUL-terminating it,
then passing it to pg_parse_lsn(), as follows?

parse_lsn(char *s, char *e, XLogRecPtr *lsn, char **c)
{
char save = *e;
- int nchars;
+ char *token_end;
+ char save_token_end;
bool success;
- unsigned hi;
- unsigned lo;

*e = '\0';
- success = (sscanf(s, "%X/%08X%n", &hi, &lo, &nchars) == 2);
- *e = save;
+ token_end = s + strcspn(s, " \t\n\r\f\v");

+ save_token_end = *token_end;
+ *token_end = '\0';
+ success = pg_parse_lsn(s, lsn);
+ *token_end = save_token_end;
+ *e = save;
if (success)
- {
- *lsn = ((XLogRecPtr) hi) << 32 | (XLogRecPtr) lo;
- *c = s + nchars;
- }
+ *c = token_end;

As for pg_parse_lsn_prefix(), it seems to accept 0/0x3000000 as 0/0,
for example. So, *if* we use pg_parse_lsn_prefix(), we'd also need to
verify that the next character is expected?

> * The new TAP tests (corrupted timeline history files for pg_rewind,
> a corrupted backup_label for pg_combinebackup) fail without the code
> change and pass with it. I could not find a way to exercise the
> server-response call sites with malformed input in a TAP test, so
> they are covered by the existing suites only; make check-world
> passes here on current master.

I'm not sure if it's really worth adding these TAP tests.

The tests cover a few manually corrupted timeline history / backup_label
cases, but there are many possible ways these files could be malformed.
I don't think testing only these specific corruption patterns adds much
value, especially since these files are normally generated by
PostgreSQL itself.

The important part of this change is to stop using sscanf() and route
the parsing through the common LSN parser. I think that's sufficient
here, so I'd prefer to keep the patch small and omit the new tests.
Thought?

Regards,

--
Fujii Masao