From 75c0939fdc2ddd1dfce7385f44f23302802d1e63 Mon Sep 17 00:00:00 2001 From: Zexin Li Date: Thu, 6 Aug 2026 02:51:40 +0000 Subject: [PATCH v2] Introduce pg_parse_lsn() to validate LSN command-line options pg_waldump (--start/--end), pg_recvlogical (--startpos/--endpos), and pg_receivewal (--endpos) parsed user-supplied WAL locations with sscanf("%X/%08X"), which accepts several forms of input that the backend's pg_lsn type rejects, and in each case proceeds with a location the user did not specify: * A first component wider than 32 bits overflows its uint32 argument, which is undefined behavior per C99 7.19.6.2p10; glibc keeps the low-order 32 bits, so --startpos 123456789/0 runs with 23456789/0. A component wider than 64 bits additionally saturates to ULONG_MAX before the truncation. * sscanf() succeeds without consuming the whole string, so trailing characters are silently ignored: 0/123456789 is read as 0/12345678 and 1/2/3 as 1/2. * The %X conversion follows strtoul()'s rules, so leading whitespace, signs, and "0x" prefixes are accepted: --endpos -1/0 runs with FFFFFFFF/0. Add pg_parse_lsn() to src/common, following the backend's pg_lsn_in_safe(): one to eight hex digits, a slash, one to eight hex digits, and nothing else. Use it for the three options above. Inputs the server accepts as pg_lsn are accepted unchanged; everything else now fails with each tool's existing "invalid WAL location" or "could not parse start/end position" error, so the error texts are unchanged. The other frontend parsers of the same shape (in pg_basebackup, pg_rewind, pg_combinebackup, and parse_manifest.c) read server-generated strings rather than command-line input and are left alone, as is the backend's pg_lsn_in_safe() itself. Add regression tests for the previously-accepted forms. Bug: #19598 Reported-by: Michael Malis Suggested-by: Fujii Masao Discussion: https://postgr.es/m/19598-aa67c8f4331611b4@postgresql.org --- src/bin/pg_basebackup/pg_receivewal.c | 6 +- src/bin/pg_basebackup/pg_recvlogical.c | 9 +-- src/bin/pg_basebackup/t/020_pg_receivewal.pl | 8 +++ src/bin/pg_basebackup/t/030_pg_recvlogical.pl | 16 +++++ src/bin/pg_waldump/pg_waldump.c | 10 +--- src/bin/pg_waldump/t/001_basic.pl | 16 +++++ src/common/Makefile | 1 + src/common/meson.build | 1 + src/common/pg_parse_lsn.c | 58 +++++++++++++++++++ src/include/common/pg_parse_lsn.h | 20 +++++++ 10 files changed, 128 insertions(+), 17 deletions(-) create mode 100644 src/common/pg_parse_lsn.c create mode 100644 src/include/common/pg_parse_lsn.h diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index 20506fc356..13bd318f67 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -30,6 +30,7 @@ #include "access/xlog_internal.h" #include "common/file_perm.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "fe_utils/option_utils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -651,8 +652,6 @@ main(int argc, char **argv) int c; int option_index; char *db_name; - uint32 hi, - lo; pg_compress_specification compression_spec; char *compression_detail = NULL; char *compression_algorithm_str = "none"; @@ -689,9 +688,8 @@ main(int argc, char **argv) basedir = pg_strdup(optarg); break; case 'E': - if (sscanf(optarg, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(optarg, &endpos)) pg_fatal("could not parse end position \"%s\"", optarg); - endpos = ((uint64) hi) << 32 | lo; break; case 'h': dbhost = pg_strdup(optarg); diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 40f6f65f75..feba45095e 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -20,6 +20,7 @@ #include "common/file_perm.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "fe_utils/option_utils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -729,8 +730,6 @@ main(int argc, char **argv) }; int c; int option_index; - uint32 hi, - lo; char *db_name; pg_logging_init(argv[0]); @@ -801,14 +800,12 @@ main(int argc, char **argv) break; /* replication options */ case 'I': - if (sscanf(optarg, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(optarg, &startpos)) pg_fatal("could not parse start position \"%s\"", optarg); - startpos = ((uint64) hi) << 32 | lo; break; case 'E': - if (sscanf(optarg, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(optarg, &endpos)) pg_fatal("could not parse end position \"%s\"", optarg); - endpos = ((uint64) hi) << 32 | lo; break; case 'o': { diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_basebackup/t/020_pg_receivewal.pl index 8da7cc86ba..9f3b0a12e9 100644 --- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl +++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl @@ -24,6 +24,14 @@ mkdir($stream_dir); # Sanity checks for command line options. $primary->command_fails(['pg_receivewal'], 'pg_receivewal needs target directory specified'); +$primary->command_fails_like( + [ 'pg_receivewal', '--endpos' => '123456789/0' ], + qr/error: could not parse end position/, + 'end position with first component wider than 32 bits'); +$primary->command_fails_like( + [ 'pg_receivewal', '--endpos' => '1/2/3' ], + qr/error: could not parse end position/, + 'end position with trailing garbage'); $primary->command_fails( [ 'pg_receivewal', diff --git a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl index 5e3e36cc4f..7893fb94ce 100644 --- a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl +++ b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl @@ -43,6 +43,22 @@ $node->command_fails( '--start', ], 'no destination file'); +$node->command_fails_like( + [ 'pg_recvlogical', '--startpos' => '123456789/0' ], + qr/error: could not parse start position/, + 'start position with first component wider than 32 bits'); +$node->command_fails_like( + [ 'pg_recvlogical', '--startpos' => '0x1/0' ], + qr/error: could not parse start position/, + 'start position with 0x prefix'); +$node->command_fails_like( + [ 'pg_recvlogical', '--endpos' => '0/123456789' ], + qr/error: could not parse end position/, + 'end position with second component wider than 32 bits'); +$node->command_fails_like( + [ 'pg_recvlogical', '--endpos' => '1/2/3' ], + qr/error: could not parse end position/, + 'end position with trailing garbage'); $node->command_ok( [ diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index ffe6e8a6bc..6f0574a876 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -27,6 +27,7 @@ #include "common/file_perm.h" #include "common/file_utils.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "common/relpath.h" #include "getopt_long.h" #include "pg_waldump.h" @@ -928,8 +929,6 @@ usage(void) int main(int argc, char **argv) { - uint32 xlogid; - uint32 xrecoff; XLogReaderState *xlogreader_state; XLogDumpPrivate private; XLogDumpConfig config; @@ -1047,13 +1046,12 @@ main(int argc, char **argv) config.filter_by_extended = true; break; case 'e': - if (sscanf(optarg, "%X/%08X", &xlogid, &xrecoff) != 2) + if (!pg_parse_lsn(optarg, &private.endptr)) { pg_log_error("invalid WAL location: \"%s\"", optarg); goto bad_argument; } - private.endptr = (uint64) xlogid << 32 | xrecoff; break; case 'f': config.follow = true; @@ -1145,14 +1143,12 @@ main(int argc, char **argv) config.filter_by_extended = true; break; case 's': - if (sscanf(optarg, "%X/%08X", &xlogid, &xrecoff) != 2) + if (!pg_parse_lsn(optarg, &private.startptr)) { pg_log_error("invalid WAL location: \"%s\"", optarg); goto bad_argument; } - else - private.startptr = (uint64) xlogid << 32 | xrecoff; break; case 't': diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl index 53b2f016b8..4fa507cfa2 100644 --- a/src/bin/pg_waldump/t/001_basic.pl +++ b/src/bin/pg_waldump/t/001_basic.pl @@ -54,6 +54,22 @@ command_fails_like( [ 'pg_waldump', '--end' => 'bad' ], qr/error: invalid WAL location/, 'invalid end LSN'); +command_fails_like( + [ 'pg_waldump', '--start' => '123456789/0' ], + qr/error: invalid WAL location/, + 'start LSN with first component wider than 32 bits'); +command_fails_like( + [ 'pg_waldump', '--start' => '0/123456789' ], + qr/error: invalid WAL location/, + 'start LSN with second component wider than 32 bits'); +command_fails_like( + [ 'pg_waldump', '--end' => '1/2/3' ], + qr/error: invalid WAL location/, + 'end LSN with trailing garbage'); +command_fails_like( + [ 'pg_waldump', '--end' => '0x1/0' ], + qr/error: invalid WAL location/, + 'end LSN with 0x prefix'); # rmgr list: If you add one to the list, consider also adding a test # case exercising the new rmgr below. diff --git a/src/common/Makefile b/src/common/Makefile index 1a2fbbe887..3404601b6b 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -70,6 +70,7 @@ OBJS_COMMON = \ percentrepl.o \ pg_get_line.o \ pg_lzcompress.o \ + pg_parse_lsn.o \ pg_prng.o \ pgfnames.o \ psprintf.o \ diff --git a/src/common/meson.build b/src/common/meson.build index 9bd55cda95..fc89a17334 100644 --- a/src/common/meson.build +++ b/src/common/meson.build @@ -24,6 +24,7 @@ common_sources = files( 'percentrepl.c', 'pg_get_line.c', 'pg_lzcompress.c', + 'pg_parse_lsn.c', 'pg_prng.c', 'pgfnames.c', 'psprintf.c', diff --git a/src/common/pg_parse_lsn.c b/src/common/pg_parse_lsn.c new file mode 100644 index 0000000000..11f5b37309 --- /dev/null +++ b/src/common/pg_parse_lsn.c @@ -0,0 +1,58 @@ +/*------------------------------------------------------------------------- + * + * pg_parse_lsn.c + * Parse a WAL location (LSN) in its text form. + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * IDENTIFICATION + * src/common/pg_parse_lsn.c + * + *------------------------------------------------------------------------- + */ + +#ifndef FRONTEND +#include "postgres.h" +#else +#include "postgres_fe.h" +#endif + +#include "common/pg_parse_lsn.h" + +/* same limit as in the backend's pg_lsn.c */ +#define MAXPG_LSNCOMPONENT 8 + +/* + * pg_parse_lsn + * + * Parse a WAL location in the "%X/%X" text form used for pg_lsn values, + * requiring one to eight hexadecimal digits in each component and nothing + * else, exactly as the backend's pg_lsn_in_safe() does. sscanf() is not + * strict enough for this purpose: its %X conversion has no field-width + * bound, so a component wider than 32 bits silently overflows a uint32 + * argument, and it also accepts leading whitespace, signs, and "0x" + * prefixes, and does not insist on consuming the whole string. + * + * Returns true and sets *result on success; returns false on syntax + * error, leaving *result unchanged. + */ +bool +pg_parse_lsn(const char *str, XLogRecPtr *result) +{ + int len1, + len2; + + len1 = strspn(str, "0123456789abcdefABCDEF"); + if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/') + return false; + + len2 = strspn(str + len1 + 1, "0123456789abcdefABCDEF"); + if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT || str[len1 + 1 + len2] != '\0') + return false; + + *result = ((uint64) strtoul(str, NULL, 16)) << 32 | + (uint32) strtoul(str + len1 + 1, NULL, 16); + + return true; +} diff --git a/src/include/common/pg_parse_lsn.h b/src/include/common/pg_parse_lsn.h new file mode 100644 index 0000000000..0a80785a4e --- /dev/null +++ b/src/include/common/pg_parse_lsn.h @@ -0,0 +1,20 @@ +/*------------------------------------------------------------------------- + * + * pg_parse_lsn.h + * Parse a WAL location (LSN) in its text form. + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/pg_parse_lsn.h + * + *------------------------------------------------------------------------- + */ +#ifndef PG_PARSE_LSN_H +#define PG_PARSE_LSN_H + +#include "access/xlogdefs.h" + +extern bool pg_parse_lsn(const char *str, XLogRecPtr *result); + +#endif /* PG_PARSE_LSN_H */ -- 2.34.1