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

On Sun, Aug 2, 2026, Michael Malis wrote:

Note the echoed value: the 9-hex-digit input 123456789 was silently
reduced to 23456789.

Thanks for the report -- reproduced on current master (03f420c37f).
Patch attached.

The != 2 guard only fires when fewer than two conversions complete,
i.e. when %X or the '/' literal fails outright, as with "ZZZ/0". It
cannot reject inputs where both conversions succeed on the wrong
bytes, which happens through three properties of sscanf():

1. The first %X carries no field width, so a component wider than 32
bits overflows the uint32 argument -- undefined behavior per C99
7.19.6.2p10; glibc keeps the low-order 32 bits, which is the mangling
you observed. A component wider than 64 bits saturates to ULONG_MAX
at the strtoul() level first, so FFFFFFFFFFFFFFFFFFFF/0 runs as
FFFFFFFF/0.

2. sscanf() succeeds without consuming the whole string. On master
the low component is %08X, so "0/123456789" stops after eight digits
and runs as 0/12345678 ("1/2/3" runs as 1/2); on 18.x, with a bare %X
there, the same input instead wraps modulo 2^32.

3. %X follows strtoul()'s subject-sequence rules, accepting leading
whitespace, signs, and "0x" prefixes: "-1/0" runs as FFFFFFFF/0.

The patch replaces the two sscanf() calls with a static helper that
follows the backend's parser for this syntax, pg_lsn_in_safe() in
src/backend/utils/adt/pg_lsn.c: strspn() over the hex charset, one to
eight digits per component, separator and terminating NUL checked by
position. The patch intentionally does not change the "invalid WAL
location" error text, the treatment of any input the server considers
valid (including 8-digit and mixed-case components), or the
already-rejected cases, so scripts matching on the error text are
unaffected.

Measured on master against a real segment. After the patch, -s/-e
accept exactly what the server accepts as pg_lsn -- one to eight hex
digits, a slash, one to eight hex digits, nothing else. Each line
shows unpatched behavior first, patched behavior second:

123456789/0: ran as 23456789/00000000; now rejected
FFFFFFFFFFFFFFFFFFFF/0: ran as FFFFFFFF/00000000; now rejected
0/123456789: ran as 0/12345678; now rejected
1/2/3: ran as 1/00000002; now rejected
0x1/0: ran as 1/00000000; now rejected
-1/0: ran as FFFFFFFF/00000000; now rejected
" 1/0": ran as 1/00000000; now rejected

"Rejected" is the existing "invalid WAL location" error; all seven
inputs are already rejected by the server when cast to pg_lsn, so the
tool and the server now agree on every string. Unchanged: valid
inputs (0/1000028, 0/0, FFFFFFFF/FFFFFFFF, abcdef/ABCDEF) parse as
before, and inputs that already failed ("bad", "ZZZ/0") keep failing
the same way.

Regression tests are included next to the existing invalid-LSN checks;
without the fix the four new cases fail. The pg_waldump TAP suite and
make check pass here.

The same pattern parses user-supplied LSNs in pg_recvlogical (-I/-E)
and pg_receivewal (-E); pg_basebackup and pg_rewind only parse
server-returned strings. I kept this patch to pg_waldump to match
the report's scope, and can send a follow-up moving the helper next
to option_parse_int() in fe_utils to cover the other two if that
seems worthwhile.

Regards,
Zexin Li

On Mon, Aug 03, 2026 03:12 AM, PG Bug reporting form <noreply@postgresql.org>
wrote:

On Tue, Aug 4, 2026 at 5:54 PM Zexin Li <lizi.openmind@gmail.com> wrote:

The same pattern parses user-supplied LSNs in pg_recvlogical (-I/-E)
and pg_receivewal (-E); pg_basebackup and pg_rewind only parse
server-returned strings. I kept this patch to pg_waldump to match
the report's scope, and can send a follow-up moving the helper next
to option_parse_int() in fe_utils to cover the other two if that
seems worthwhile.

I think it would be better to improve pg_recvlogical and pg_receivewal as well,
not just pg_waldump, by introducing a common LSN parsing helper in,
for example, src/common. That would let frontend tools share exactly
the same LSN syntax checks.

pg_basebackup, pg_verifybackup, pg_rewind, and pg_combinebackup also parse LSN
but are a bit different, since they mostly parse LSNs from server responses,
backup manifests, backup_label files, or timeline history files rather than
direct command-line input. So they're less likely to see arbitrary invalid LSNs
from users.

Still, if we introduce a common LSN parser, it seems worth considering
converting those existing sscanf("%X/%08X") call sites as well. That would
make malformed metadata fail earlier and avoid having several slightly
different LSN parsers in frontend code. This should be done as a separate
patch from the pg_waldump/pg_recvlogical/pg_receivewal improvement,
though.

BTW, at least for me this looks more like an improvement than a bug fix.
So I think it should target v20devel.

Regards,

--
Fujii Masao

On Wed, Aug 5, 2026, Fujii Masao wrote:

I think it would be better to improve pg_recvlogical and pg_receivewal
as well, not just pg_waldump, by introducing a common LSN parsing
helper in, for example, src/common. That would let frontend tools
share exactly the same LSN syntax checks.

Agreed -- v2 attached, done that way.

The helper is pg_parse_lsn() in the new src/common/pg_parse_lsn.c,
with the same rules as the backend's pg_lsn_in_safe(): one to eight
hex digits, a slash, one to eight hex digits, and nothing else.
pg_waldump's static helper from v1 moves there, and pg_recvlogical
(-I/-E) and pg_receivewal (-E) now go through it as well.

Three choices worth calling out:

* The helper only parses and returns bool; each tool keeps its own
existing error message ("invalid WAL location" in pg_waldump,
"could not parse start/end position" in the other two), so no error
text changes anywhere. This follows the existing split between
strtoint() in src/common and option_parse_int() in fe_utils.

* No endptr-style variant yet. The three command-line options all
want whole-string parsing. Among the call sites for the separate
patch you describe, pg_rewind's timeline.c and pg_combinebackup's
backup_label.c parse an LSN as a prefix of a longer line, so that
patch will want a second entry point taking an endptr; I did not
add an API with no in-tree caller here. One more data point for
it: parse_manifest.c contains one more sscanf of the same shape,
and it lives in src/common itself, out of reach of fe_utils code
-- which also argues for src/common as the helper's home.

* The backend's pg_lsn_in_safe() is left untouched for now.

The behavior change is confined to the three options: strings the
server rejects as pg_lsn (overlong components, trailing garbage,
leading whitespace, signs, 0x prefixes) now fail with each tool's
existing error instead of running with a mangled location. Strings
the server accepts parse exactly as before; I re-ran the v1 input
matrix against all three tools to check both directions.

The new TAP cases for pg_recvlogical and pg_receivewal fail without
the code change and pass with it; the pg_waldump cases from v1 are
kept. make check-world, a full meson build, and git am on current
master are all clean here.

BTW, at least for me this looks more like an improvement than a bug fix.
So I think it should target v20devel.

Makes sense. I'll register the patch in the September commitfest.

I'd appreciate any feedback .

Regards,
Zexin Li

On Wed, Aug 05, 2026 03:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Aug 4, 2026 at 5:54 PM Zexin Li <lizi.openmind@gmail.com> wrote:

The same pattern parses user-supplied LSNs in pg_recvlogical (-I/-E)
and pg_receivewal (-E); pg_basebackup and pg_rewind only parse
server-returned strings. I kept this patch to pg_waldump to match
the report's scope, and can send a follow-up moving the helper next
to option_parse_int() in fe_utils to cover the other two if that
seems worthwhile.

I think it would be better to improve pg_recvlogical and pg_receivewal as
well,
not just pg_waldump, by introducing a common LSN parsing helper in,
for example, src/common. That would let frontend tools share exactly
the same LSN syntax checks.

pg_basebackup, pg_verifybackup, pg_rewind, and pg_combinebackup also parse
LSN
but are a bit different, since they mostly parse LSNs from server
responses,
backup manifests, backup_label files, or timeline history files rather

than

direct command-line input. So they're less likely to see arbitrary invalid
LSNs
from users.

Still, if we introduce a common LSN parser, it seems worth considering
converting those existing sscanf("%X/%08X") call sites as well. That would
make malformed metadata fail earlier and avoid having several slightly
different LSN parsers in frontend code. This should be done as a separate
patch from the pg_waldump/pg_recvlogical/pg_receivewal improvement,
though.

BTW, at least for me this looks more like an improvement than a bug fix.
So I think it should target v20devel.

Regards,

--
Fujii Masao

On Wed, Aug 05, 2026 03:03 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Aug 4, 2026 at 5:54 PM Zexin Li <lizi.openmind@gmail.com> wrote:

The same pattern parses user-supplied LSNs in pg_recvlogical (-I/-E)
and pg_receivewal (-E); pg_basebackup and pg_rewind only parse
server-returned strings. I kept this patch to pg_waldump to match
the report's scope, and can send a follow-up moving the helper next
to option_parse_int() in fe_utils to cover the other two if that
seems worthwhile.

I think it would be better to improve pg_recvlogical and pg_receivewal as
well,
not just pg_waldump, by introducing a common LSN parsing helper in,
for example, src/common. That would let frontend tools share exactly
the same LSN syntax checks.

pg_basebackup, pg_verifybackup, pg_rewind, and pg_combinebackup also parse
LSN
but are a bit different, since they mostly parse LSNs from server
responses,
backup manifests, backup_label files, or timeline history files rather than
direct command-line input. So they're less likely to see arbitrary invalid
LSNs
from users.

Still, if we introduce a common LSN parser, it seems worth considering
converting those existing sscanf("%X/%08X") call sites as well. That would
make malformed metadata fail earlier and avoid having several slightly
different LSN parsers in frontend code. This should be done as a separate
patch from the pg_waldump/pg_recvlogical/pg_receivewal improvement,
though.

BTW, at least for me this looks more like an improvement than a bug fix.
So I think it should target v20devel.

Regards,

--
Fujii Masao

On Fri, Aug 7, 2026 at 10:58 AM Zexin Li <lizi.openmind@gmail.com> wrote:

The helper is pg_parse_lsn() in the new src/common/pg_parse_lsn.c,
with the same rules as the backend's pg_lsn_in_safe(): one to eight
hex digits, a slash, one to eight hex digits, and nothing else.
pg_waldump's static helper from v1 moves there, and pg_recvlogical
(-I/-E) and pg_receivewal (-E) now go through it as well.

* The backend's pg_lsn_in_safe() is left untouched for now.

Thanks for updating the patch!

Attached is a revised version. It keeps the v2 approach of adding
pg_parse_lsn() in src/common and using it for user-supplied LSN
command-line options in pg_waldump, pg_recvlogical, and
pg_receivewal.

The main change from v2 is that pg_lsn_in_safe() now also uses
pg_parse_lsn(), leaving only the backend-specific soft-error handling
there. This avoids duplicating the LSN syntax checks.

Thought?

Regards,

--
Fujii Masao

Hi,

On Mon, 10 Aug 2026 at 11:17, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Aug 7, 2026 at 10:58 AM Zexin Li <lizi.openmind@gmail.com> wrote:

The helper is pg_parse_lsn() in the new src/common/pg_parse_lsn.c,
with the same rules as the backend's pg_lsn_in_safe(): one to eight
hex digits, a slash, one to eight hex digits, and nothing else.
pg_waldump's static helper from v1 moves there, and pg_recvlogical
(-I/-E) and pg_receivewal (-E) now go through it as well.

* The backend's pg_lsn_in_safe() is left untouched for now.

Thanks for updating the patch!

Attached is a revised version. It keeps the v2 approach of adding
pg_parse_lsn() in src/common and using it for user-supplied LSN
command-line options in pg_waldump, pg_recvlogical, and
pg_receivewal.

The main change from v2 is that pg_lsn_in_safe() now also uses
pg_parse_lsn(), leaving only the backend-specific soft-error handling
there. This avoids duplicating the LSN syntax checks.

Thought?

I reviewed v3, and it looks good to me.
In parallel, I had reported and patched the same sscanf("%X/%08X")
issue in src/common/parse_manifest.c [0]/messages/by-id/CAJTYsWXieRHb-ooV2XfjHAtBsS7+P5La_-o8-Cqi15CNDhh9hQ@mail.gmail.com

Once this is committed, I'll rebase that patch onto it and post a new
version on the earlier thread.

Regards,
Ayush

[0]: /messages/by-id/CAJTYsWXieRHb-ooV2XfjHAtBsS7+P5La_-o8-Cqi15CNDhh9hQ@mail.gmail.com
/messages/by-id/CAJTYsWXieRHb-ooV2XfjHAtBsS7+P5La_-o8-Cqi15CNDhh9hQ@mail.gmail.com

On Sun, Aug 10, 2026, Fujii Masao wrote:

The main change from v2 is that pg_lsn_in_safe() now also uses
pg_parse_lsn(), leaving only the backend-specific soft-error handling
there. This avoids duplicating the LSN syntax checks.

Thank you for the review. v3 looks good to me.

Regards,
Zexin Li

On Mon, Aug 10, 2026 02:47 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Aug 11, 2026 at 9:41 AM Zexin Li <lizi.openmind@gmail.com> wrote:

On Sun, Aug 10, 2026, Fujii Masao wrote:

The main change from v2 is that pg_lsn_in_safe() now also uses
pg_parse_lsn(), leaving only the backend-specific soft-error handling
there. This avoids duplicating the LSN syntax checks.

Thank you for the review. v3 looks good to me.

I've pushed the patch. Thanks!

Regards,

--
Fujii Masao

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. The backend's copies of these parsers are left untouched,
and parse_manifest.c is already being handled by Ayush's patch.

* 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.

* Malformed metadata now fails with each tool's existing error
instead of silently proceeding with a different location: components
wider than eight hex digits used to wrap around or be truncated, and
whitespace, signs, "0x" prefixes, and trailing characters used to be
consumed or ignored. Two error-path details change: pg_rewind's
history-file parser now requires the switchpoint to be followed by
whitespace or end of line, where trailing characters used to be
ignored, and an overlong second component in a backup_label LSN now
fails pg_combinebackup's "could not parse" check rather than its
"improper terminator" check.

* 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'd appreciate any feedback .

Regards,
Zexin Li

On Wed, Aug 12, 2026 05:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Aug 14, 2026 at 3:33 PM Zexin Li <lizi.openmind@gmail.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

On Fri, Aug 14, 2026, Fujii Masao wrote:

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?

You're right, that is better. v2 attached does it that way in
backup_label.c, and applies the same approach in pg_rewind's
timeline.c, so the patch no longer touches src/common at all.

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?

Right about the helper on its own. In v1 both call sites checked the
next character -- in backup_label.c, the terminator check that was
already there -- so neither tool accepted that input. In v2 the
whole token goes to pg_parse_lsn(), so there is no such check left
for a caller to get wrong.

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

Agreed. Dropped in v2.

make check-world passes here.
I'd appreciate any feedback.

Regards,
Zexin Li

On Mon, Aug 17, 2026 at 12:23 PM Zexin Li <lizi.openmind@gmail.com> wrote:

You're right, that is better. v2 attached does it that way in
backup_label.c, and applies the same approach in pg_rewind's
timeline.c, so the patch no longer touches src/common at all.

Thanks for updating the patch! It looks good to me.
Barring any objections, I will commit it.

Regards,

--
Fujii Masao

On Thu, Aug 20, 2026 at 2:57 AM Fujii Masao <masao.fujii@gmail.com> wrote:

Thanks for updating the patch! It looks good to me.
Barring any objections, I will commit it.

I've pushed the patch with a small adjustment. Thanks!

Regards,

--
Fujii Masao