On 1/24/25 01:44, Michael Paquier wrote:
On Thu, Jan 23, 2025 at 02:53:39PM +0000, David Steele wrote:
I discovered this while testing on Postgres versions < 12 where
The tests are probably excessive but I needed something to show that the
verification works as expected.
Even with your patch, specifying an incorrect name results in a
complaint about a timeline of 0. Wouldn't it be better to strengthen
the parsing in check_recovery_target_timeline() and/or the error
message reported?
I attached the wrong patch. Oops!
Regards,
-David
From 453c8937868e0c70e4bfecf0a39bf8052e379efe Mon Sep 17 00:00:00 2001
From: David Steele <da...@pgmasters.net>
Date: Thu, 23 Jan 2025 14:40:43 +0000
Subject: Improve verification of recovery_target_timeline GUC.
Currently check_recovery_target_timeline() converts any value that is not
current, latest, or a valid integer to 0. So for example:
recovery_target_timeline = 'currrent'
results in the following error:
FATAL: 22023: recovery target timeline 0 does not exist
Since there is no range checking for uint32 (but there is a cast from unsigned
long) this setting:
recovery_target_timeline = '9999999999'
results in the following error:
FATAL: 22023: recovery target timeline 1410065407 does not exist
Improve by adding endptr checking to catch conversion errors and add range
checking to
exclude values < 2 and greater than UINT_MAX.
---
src/backend/access/transam/xlogrecovery.c | 14 ++++-
src/test/recovery/t/003_recovery_targets.pl | 60 +++++++++++++++++++++
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index cf2b007806f..1941e3061f4 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -4973,15 +4973,25 @@ check_recovery_target_timeline(char **newval, void
**extra, GucSource source)
rttg = RECOVERY_TARGET_TIMELINE_LATEST;
else
{
+ char *endp;
+ unsigned long long timeline;
rttg = RECOVERY_TARGET_TIMELINE_NUMERIC;
errno = 0;
- strtoul(*newval, NULL, 0);
- if (errno == EINVAL || errno == ERANGE)
+ timeline = strtoull(*newval, &endp, 0);
+
+ if (*endp != '\0' || errno == EINVAL || errno == ERANGE)
{
GUC_check_errdetail("\"recovery_target_timeline\" is
not a valid number.");
return false;
}
+
+ if (timeline < 2 || timeline > UINT_MAX)
+ {
+ GUC_check_errdetail(
+ "\"recovery_target_timeline\" must be between 2
and 4,294,967,295.");
+ return false;
+ }
}
myextra = (RecoveryTargetTimeLineGoal *) guc_malloc(ERROR,
sizeof(RecoveryTargetTimeLineGoal));
diff --git a/src/test/recovery/t/003_recovery_targets.pl
b/src/test/recovery/t/003_recovery_targets.pl
index 0ae2e982727..0424ff4ad22 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -187,4 +187,64 @@ ok( $logfile =~
qr/FATAL: .* recovery ended before configured recovery target was
reached/,
'recovery end before target reached is a fatal error');
+# Invalid timeline target
+$node_standby = PostgreSQL::Test::Cluster->new('standby_9');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$node_standby->append_conf(
+ 'postgresql.conf', "recovery_target_timeline = 'bogus'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid recovery startup fails');
+
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/is not a valid number/,
+ 'invalid target timelime');
+
+# Timeline target out of min range
+$node_standby = PostgreSQL::Test::Cluster->new('standby_10');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$node_standby->append_conf(
+ 'postgresql.conf', "recovery_target_timeline = '1'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid recovery startup fails');
+
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/must be between 2 and 4,294,967,295/,
+ 'target timelime out of min range');
+
+# Timeline target out of max range
+$node_standby = PostgreSQL::Test::Cluster->new('standby_11');
+$node_standby->init_from_backup($node_primary, 'my_backup',
+ has_restoring => 1);
+$node_standby->append_conf(
+ 'postgresql.conf', "recovery_target_timeline = '4294967296'");
+
+$res = run_log(
+ [
+ 'pg_ctl',
+ '--pgdata' => $node_standby->data_dir,
+ '--log' => $node_standby->logfile,
+ 'start',
+ ]);
+ok(!$res, 'invalid recovery startup fails');
+
+$logfile = slurp_file($node_standby->logfile());
+ok($logfile =~ qr/must be between 2 and 4,294,967,295/,
+ 'target timelime out of max range');
+
done_testing();
--
2.34.1
