From 4d28b9e2e68ce0561111d770218c9d80e869039a Mon Sep 17 00:00:00 2001 From: Zexin Li Date: Fri, 14 Aug 2026 00:37:28 +0000 Subject: [PATCH v1] Use pg_parse_lsn() for server-supplied LSNs Commit d6bf0ab170 introduced pg_parse_lsn() to validate LSNs given on the command line of pg_waldump, pg_recvlogical, and pg_receivewal. The remaining frontend sscanf("%X/%08X") call sites parse LSNs that arrive in server responses, timeline history files, and backup_label files. sscanf() accepts several forms that pg_lsn input rejects and can silently continue with a different location than the input text: a first component wider than eight hex digits wraps around, a wider second component is truncated, and leading whitespace, signs, "0x" prefixes, and trailing characters are consumed or ignored. Convert those call sites as well. Call sites that read a location out of a longer line need to keep parsing where the location ended, so add pg_parse_lsn_prefix(), which reports the first character after the location instead of requiring the string to end there, and reimplement pg_parse_lsn() on top of it. 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. 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. Author: Zexin Li --- src/bin/pg_basebackup/pg_basebackup.c | 16 +--- src/bin/pg_basebackup/receivelog.c | 8 +- src/bin/pg_basebackup/streamutil.c | 24 ++--- src/bin/pg_combinebackup/backup_label.c | 12 +-- src/bin/pg_combinebackup/t/005_integrity.pl | 26 +++++ src/bin/pg_rewind/libpq_source.c | 7 +- src/bin/pg_rewind/meson.build | 1 + src/bin/pg_rewind/t/012_timeline_history.pl | 100 ++++++++++++++++++++ src/bin/pg_rewind/timeline.c | 22 +++-- src/common/pg_parse_lsn.c | 53 +++++++++-- src/include/common/pg_parse_lsn.h | 2 + 11 files changed, 206 insertions(+), 65 deletions(-) create mode 100644 src/bin/pg_rewind/t/012_timeline_history.pl diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 12fc752bff5..c3b87a19e76 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -32,6 +32,7 @@ #include "common/file_perm.h" #include "common/file_utils.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "fe_utils/option_utils.h" #include "fe_utils/recovery_gen.h" #include "getopt_long.h" @@ -482,17 +483,14 @@ reached_end_position(XLogRecPtr segendpos, uint32 timeline, { ssize_t nread; char xlogend[64] = {0}; - uint32 hi, - lo; nread = read(bgpipe[0], xlogend, sizeof(xlogend) - 1); if (nread < 0) pg_fatal("could not read from ready pipe: %m"); - if (sscanf(xlogend, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(xlogend, &xlogendptr)) pg_fatal("could not parse write-ahead log location \"%s\"", xlogend); - xlogendptr = ((uint64) hi) << 32 | lo; has_xlogendptr = 1; /* @@ -620,8 +618,6 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier, int wal_compress_level) { logstreamer_param *param; - uint32 hi, - lo; char statusdir[MAXPGPATH]; param = pg_malloc0_object(logstreamer_param); @@ -631,10 +627,9 @@ StartLogStreamer(char *startpos, uint32 timeline, char *sysidentifier, param->wal_compress_level = wal_compress_level; /* Convert the starting position */ - if (sscanf(startpos, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(startpos, ¶m->startptr)) pg_fatal("could not parse write-ahead log location \"%s\"", startpos); - param->startptr = ((uint64) hi) << 32 | lo; /* Round off to even segment position */ param->startptr -= XLogSegmentOffset(param->startptr, WalSegSz); @@ -2216,8 +2211,6 @@ BaseBackup(char *compression_algorithm, char *compression_detail, * casting to a different size on WIN64. */ intptr_t bgchild_handle = bgchild; - uint32 hi, - lo; #endif if (verbose) @@ -2243,10 +2236,9 @@ BaseBackup(char *compression_algorithm, char *compression_detail, * value directly in the variable, and then set the flag that says * it's there. */ - if (sscanf(xlogend, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(xlogend, &xlogendptr)) pg_fatal("could not parse write-ahead log location \"%s\"", xlogend); - xlogendptr = ((uint64) hi) << 32 | lo; InterlockedIncrement(&has_xlogendptr); /* First wait for the thread to exit */ diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c index faa60711b1b..77a2b4458b3 100644 --- a/src/bin/pg_basebackup/receivelog.c +++ b/src/bin/pg_basebackup/receivelog.c @@ -20,6 +20,7 @@ #include "access/xlog_internal.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "libpq-fe.h" #include "libpq/protocol.h" #include "receivelog.h" @@ -704,9 +705,6 @@ error: static bool ReadEndOfStreamingResult(PGresult *res, XLogRecPtr *startpos, uint32 *timeline) { - uint32 startpos_xlogid, - startpos_xrecoff; - /*---------- * The result set consists of one row and two columns, e.g: * @@ -727,14 +725,12 @@ ReadEndOfStreamingResult(PGresult *res, XLogRecPtr *startpos, uint32 *timeline) } *timeline = atoi(PQgetvalue(res, 0, 0)); - if (sscanf(PQgetvalue(res, 0, 1), "%X/%08X", &startpos_xlogid, - &startpos_xrecoff) != 2) + if (!pg_parse_lsn(PQgetvalue(res, 0, 1), startpos)) { pg_log_error("could not parse next timeline's starting point \"%s\"", PQgetvalue(res, 0, 1)); return false; } - *startpos = ((uint64) startpos_xlogid << 32) | startpos_xrecoff; return true; } diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c index 8fcd690f155..e3c923cd317 100644 --- a/src/bin/pg_basebackup/streamutil.c +++ b/src/bin/pg_basebackup/streamutil.c @@ -21,6 +21,7 @@ #include "common/connect.h" #include "common/file_perm.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "common/string.h" #include "datatype/timestamp.h" #include "port/pg_bswap.h" @@ -410,8 +411,6 @@ RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli, XLogRecPtr *startpos, char **db_name) { PGresult *res; - uint32 hi, - lo; /* Check connection existence */ Assert(conn != NULL); @@ -445,7 +444,7 @@ RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli, /* Get LSN start position if necessary */ if (startpos != NULL) { - if (sscanf(PQgetvalue(res, 0, 2), "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(PQgetvalue(res, 0, 2), startpos)) { pg_log_error("could not parse write-ahead log location \"%s\"", PQgetvalue(res, 0, 2)); @@ -453,7 +452,6 @@ RunIdentifySystem(PGconn *conn, char **sysid, TimeLineID *starttli, PQclear(res); return false; } - *startpos = ((uint64) hi) << 32 | lo; } /* Get database name, only available in 9.4 and newer versions */ @@ -547,19 +545,13 @@ GetSlotInformation(PGconn *conn, const char *slot_name, } /* restart LSN */ - if (!PQgetisnull(res, 0, 1)) + if (!PQgetisnull(res, 0, 1) && + !pg_parse_lsn(PQgetvalue(res, 0, 1), &lsn_loc)) { - uint32 hi, - lo; - - if (sscanf(PQgetvalue(res, 0, 1), "%X/%08X", &hi, &lo) != 2) - { - pg_log_error("could not parse restart_lsn \"%s\" for replication slot \"%s\"", - PQgetvalue(res, 0, 1), slot_name); - PQclear(res); - return false; - } - lsn_loc = ((uint64) hi) << 32 | lo; + pg_log_error("could not parse restart_lsn \"%s\" for replication slot \"%s\"", + PQgetvalue(res, 0, 1), slot_name); + PQclear(res); + return false; } /* current TLI */ diff --git a/src/bin/pg_combinebackup/backup_label.c b/src/bin/pg_combinebackup/backup_label.c index b757e772b92..ca9ed2e7b4a 100644 --- a/src/bin/pg_combinebackup/backup_label.c +++ b/src/bin/pg_combinebackup/backup_label.c @@ -17,6 +17,7 @@ #include "backup_label.h" #include "common/file_perm.h" #include "common/logging.h" +#include "common/pg_parse_lsn.h" #include "write_manifest.h" static int get_eol_offset(StringInfo buf); @@ -242,20 +243,15 @@ static bool parse_lsn(char *s, char *e, XLogRecPtr *lsn, char **c) { char save = *e; - int nchars; bool success; - unsigned hi; - unsigned lo; + const char *end; *e = '\0'; - success = (sscanf(s, "%X/%08X%n", &hi, &lo, &nchars) == 2); + success = pg_parse_lsn_prefix(s, lsn, &end); *e = save; if (success) - { - *lsn = ((XLogRecPtr) hi) << 32 | (XLogRecPtr) lo; - *c = s + nchars; - } + *c = unconstify(char *, end); return success; } diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl index 9e1af2a7a7f..546d70c6d11 100644 --- a/src/bin/pg_combinebackup/t/005_integrity.pl +++ b/src/bin/pg_combinebackup/t/005_integrity.pl @@ -100,6 +100,7 @@ $node2->command_ok( # Result directory. my $resultpath = $node1->backup_dir . '/result'; +my $badlsnpath = $node1->backup_dir . '/badlsn'; # Can't combine 2 full backups. $node1->command_fails_like( @@ -208,5 +209,30 @@ $node1->command_fails_like( qr/starts at LSN.*but expected/, "can't combine synthetic backup with included incremental"); +# A start location whose first component is wider than 32 bits must be +# rejected; it used to wrap around silently. +my $labelpath = $backup2path . '/backup_label'; +my $origlabel = slurp_file($labelpath); +my $badlabel = $origlabel; +$badlabel =~ + s{^START WAL LOCATION: [0-9A-F]+/}{START WAL LOCATION: 123456789/}m; +open my $lfh, '>', $labelpath or die "$labelpath: $!"; +print $lfh $badlabel; +close $lfh; +$node1->command_fails_like( + [ + 'pg_combinebackup', $backup1path, $backup2path, + '--output' => $badlsnpath, + $mode, + ], + qr/could not parse START WAL LOCATION/, + "can't combine a backup whose start location is out of range"); +rmtree($badlsnpath) if -d $badlsnpath; + +# Restore the original file. +open $lfh, '>', $labelpath or die "$labelpath: $!"; +print $lfh $origlabel; +close $lfh; + # OK, that's all. done_testing(); diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c index 216755b3ddb..abfbd312580 100644 --- a/src/bin/pg_rewind/libpq_source.c +++ b/src/bin/pg_rewind/libpq_source.c @@ -11,6 +11,7 @@ #include "catalog/pg_type_d.h" #include "common/connect.h" +#include "common/pg_parse_lsn.h" #include "file_ops.h" #include "filemap.h" #include "lib/stringinfo.h" @@ -209,17 +210,13 @@ libpq_get_current_wal_insert_lsn(rewind_source *source) { PGconn *conn = ((libpq_source *) source)->conn; XLogRecPtr result; - uint32 hi; - uint32 lo; char *val; val = run_simple_query(conn, "SELECT pg_current_wal_insert_lsn()"); - if (sscanf(val, "%X/%08X", &hi, &lo) != 2) + if (!pg_parse_lsn(val, &result)) pg_fatal("unrecognized result \"%s\" for current WAL insert location", val); - result = ((uint64) hi) << 32 | lo; - pg_free(val); return result; diff --git a/src/bin/pg_rewind/meson.build b/src/bin/pg_rewind/meson.build index 52a6ab0a515..a328ab3e6b8 100644 --- a/src/bin/pg_rewind/meson.build +++ b/src/bin/pg_rewind/meson.build @@ -45,6 +45,7 @@ tests += { 't/009_growing_files.pl', 't/010_keep_recycled_wals.pl', 't/011_wal_copy.pl', + 't/012_timeline_history.pl', ], }, } diff --git a/src/bin/pg_rewind/t/012_timeline_history.pl b/src/bin/pg_rewind/t/012_timeline_history.pl new file mode 100644 index 00000000000..f967dcfca4c --- /dev/null +++ b/src/bin/pg_rewind/t/012_timeline_history.pl @@ -0,0 +1,100 @@ + +# Copyright (c) 2026, PostgreSQL Global Development Group + +# +# Test that a malformed switchpoint in a timeline history file is rejected +# rather than silently misinterpreted. +# +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use Test::More; + +use FindBin; +use lib $FindBin::RealBin; + +use RewindTest; + +RewindTest::setup_cluster('history'); +RewindTest::start_primary(); +RewindTest::create_standby('history'); +RewindTest::promote_standby(); + +my $primary_pgdata = $node_primary->data_dir; +my $standby_pgdata = $node_standby->data_dir; + +$node_standby->stop; +$node_primary->stop; + +# The history file of the promoted standby is read by pg_rewind to find the +# point where the two servers diverged. +my $histfile = "$standby_pgdata/pg_wal/00000002.history"; +my $orig = slurp_file($histfile); + +sub write_history +{ + my $contents = shift; + + open my $fh, '>', $histfile + or BAIL_OUT("could not write \"$histfile\": $!"); + print $fh $contents; + close $fh; + return; +} + +# A switchpoint whose first component is wider than 32 bits must not be +# accepted; the value used to wrap around silently. +write_history("1\t123456789/0\tno recovery target specified\n"); +command_fails_like( + [ + 'pg_rewind', + '--dry-run', + '--source-pgdata' => $standby_pgdata, + '--target-pgdata' => $primary_pgdata, + '--no-sync', + ], + qr/error: syntax error in history file/, + 'switchpoint with first component wider than 32 bits'); + +# Likewise for a second component that is too wide, which used to be +# truncated to its first eight digits. +write_history("1\t0/123456789\tno recovery target specified\n"); +command_fails_like( + [ + 'pg_rewind', + '--dry-run', + '--source-pgdata' => $standby_pgdata, + '--target-pgdata' => $primary_pgdata, + '--no-sync', + ], + qr/error: syntax error in history file/, + 'switchpoint with second component wider than 32 bits'); + +# A switchpoint written with a "0x" prefix must also be rejected; sscanf's +# %X used to consume the prefix silently. +write_history("1\t0/0x3000000\tno recovery target specified\n"); +command_fails_like( + [ + 'pg_rewind', + '--dry-run', + '--source-pgdata' => $standby_pgdata, + '--target-pgdata' => $primary_pgdata, + '--no-sync', + ], + qr/error: syntax error in history file/, + 'switchpoint with 0x prefix'); + +# The unmodified file is still accepted, so the failures above are caused by +# the switchpoint and nothing else. +write_history($orig); +command_ok( + [ + 'pg_rewind', + '--dry-run', + '--source-pgdata' => $standby_pgdata, + '--target-pgdata' => $primary_pgdata, + '--no-sync', + ], + 'unmodified history file is accepted'); + +done_testing(); diff --git a/src/bin/pg_rewind/timeline.c b/src/bin/pg_rewind/timeline.c index dda06eaa0bc..957f1c88fcf 100644 --- a/src/bin/pg_rewind/timeline.c +++ b/src/bin/pg_rewind/timeline.c @@ -10,6 +10,7 @@ #include "postgres_fe.h" #include "access/timeline.h" +#include "common/pg_parse_lsn.h" #include "pg_rewind.h" /* @@ -45,9 +46,9 @@ rewind_parseTimeLineHistory(char *buffer, TimeLineID targetTLI, int *nentries) { char *ptr; TimeLineID tli; - uint32 switchpoint_hi; - uint32 switchpoint_lo; - int nfields; + XLogRecPtr switchpoint; + const char *lsnend; + int nchars; fline = bufptr; while (*bufptr && *bufptr != '\n') @@ -66,16 +67,21 @@ rewind_parseTimeLineHistory(char *buffer, TimeLineID targetTLI, int *nentries) if (*ptr == '\0' || *ptr == '#') continue; - nfields = sscanf(fline, "%u\t%X/%08X", &tli, &switchpoint_hi, &switchpoint_lo); - - if (nfields < 1) + if (sscanf(fline, "%u%n", &tli, &nchars) != 1) { /* expect a numeric timeline ID as first field of line */ pg_log_error("syntax error in history file: %s", fline); pg_log_error_detail("Expected a numeric timeline ID."); exit(1); } - if (nfields != 3) + + /* the switchpoint location follows, separated by whitespace */ + ptr = fline + nchars; + while (isspace((unsigned char) *ptr)) + ptr++; + + if (!pg_parse_lsn_prefix(ptr, &switchpoint, &lsnend) || + (*lsnend != '\0' && !isspace((unsigned char) *lsnend))) { pg_log_error("syntax error in history file: %s", fline); pg_log_error_detail("Expected a write-ahead log switchpoint location."); @@ -96,7 +102,7 @@ rewind_parseTimeLineHistory(char *buffer, TimeLineID targetTLI, int *nentries) entry = &entries[nlines - 1]; entry->tli = tli; entry->begin = prevend; - entry->end = ((uint64) (switchpoint_hi)) << 32 | (uint64) switchpoint_lo; + entry->end = switchpoint; prevend = entry->end; /* we ignore the remainder of each line */ diff --git a/src/common/pg_parse_lsn.c b/src/common/pg_parse_lsn.c index b6a83b57216..1979fa02387 100644 --- a/src/common/pg_parse_lsn.c +++ b/src/common/pg_parse_lsn.c @@ -23,20 +23,21 @@ #define MAXPG_LSNCOMPONENT 8 /* - * pg_parse_lsn + * pg_parse_lsn_prefix * * 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. Unlike sscanf(), this rejects components longer than eight - * hexadecimal digits, leading whitespace, signs, "0x" prefixes, and - * trailing characters. + * requiring one to eight hexadecimal digits in each component. Unlike + * sscanf(), this rejects components longer than eight hexadecimal digits, + * leading whitespace, signs, and "0x" prefixes. * - * Returns true and sets *result on success; returns false on syntax - * error, leaving *result unchanged. + * Returns true on success, setting *result to the location and *endptr to + * the first character after it. Returns false on syntax error, leaving + * both unchanged. */ bool -pg_parse_lsn(const char *str, XLogRecPtr *result) +pg_parse_lsn_prefix(const char *str, XLogRecPtr *result, const char **endptr) { + char buf[MAXPG_LSNCOMPONENT + 1]; size_t len1, len2; @@ -45,11 +46,43 @@ pg_parse_lsn(const char *str, XLogRecPtr *result) return false; len2 = strspn(str + len1 + 1, "0123456789abcdefABCDEF"); - if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT || str[len1 + 1 + len2] != '\0') + if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT) return false; + /* + * Decode the second component from a bounded copy: on the original + * string, strtoul() would accept a "0x" prefix that the check above did + * not. (The first component ends at the '/'.) + */ + memcpy(buf, str + len1 + 1, len2); + buf[len2] = '\0'; + *result = ((uint64) strtoul(str, NULL, 16)) << 32 | - (uint32) strtoul(str + len1 + 1, NULL, 16); + (uint32) strtoul(buf, NULL, 16); + *endptr = str + len1 + 1 + len2; + + return true; +} + +/* + * pg_parse_lsn + * + * Same as pg_parse_lsn_prefix(), but the whole string has to be a WAL + * location. + * + * Returns true and sets *result on success; returns false on syntax + * error, leaving *result unchanged. + */ +bool +pg_parse_lsn(const char *str, XLogRecPtr *result) +{ + XLogRecPtr lsn; + const char *end; + + if (!pg_parse_lsn_prefix(str, &lsn, &end) || *end != '\0') + return false; + + *result = lsn; return true; } diff --git a/src/include/common/pg_parse_lsn.h b/src/include/common/pg_parse_lsn.h index 0a80785a4e9..740ce6e1226 100644 --- a/src/include/common/pg_parse_lsn.h +++ b/src/include/common/pg_parse_lsn.h @@ -16,5 +16,7 @@ #include "access/xlogdefs.h" extern bool pg_parse_lsn(const char *str, XLogRecPtr *result); +extern bool pg_parse_lsn_prefix(const char *str, XLogRecPtr *result, + const char **endptr); #endif /* PG_PARSE_LSN_H */ -- 2.34.1