From: David Steele <david@pgbackrest.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve verification of recovery_target_timeline GUC.
Date: Fri, 24 Jan 2025 13:36:45 +0000 (UTC)
Message-ID: <a17d1bf9-8123-4b86-9c84-5b7526cd70fd@pgbackrest.org> (raw)
In-Reply-To: <Z5M2tgQUMgCkyrKJ@paquier.xyz>
References: <e5d472c7-e9be-4710-8dc4-ebe721b62cea@pgbackrest.org>
<Z5M2tgQUMgCkyrKJ@paquier.xyz>
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
Attachments:timeline-check-v1.patch (text/plain, 4.2K) download
From 453c8937868e0c70e4bfecf0a39bf8052e379efe Mon Sep 17 00:00:00 2001
From: David Steele <david@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.cindex 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.plindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: david@pgbackrest.org, michael@paquier.xyz
Subject: Re: Improve verification of recovery_target_timeline GUC.
In-Reply-To: <a17d1bf9-8123-4b86-9c84-5b7526cd70fd@pgbackrest.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox