pg_amcheck: Fix block number parsing on command line
It seems to me that when using the pg_amcheck --startblock and
--endblock options on platforms where sizeof(long) == 4, you cannot
specify higher block numbers (unless you do tricks with negative
numbers). The attached patch should fix this by using strtoul() instead
of strtol(). I also tightened up the number scanning a bit in other
ways, similar to the code in other frontend utilities. I know some
people have been working on tightening all this up. Please check that
it's up to speed.
Attachments:
0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patchtext/plain; charset=UTF-8; name=0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch; x-mac-creator=0; x-mac-type=0Download
From d8bcc7cab6000acdaab69a81a57d7d6d41ae7bd5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 22 Jul 2021 16:49:37 +0200
Subject: [PATCH] pg_amcheck: Fix block number parsing on command line
The previous code wouldn't handle higher block numbers on systems
where sizeof(long) == 4.
---
src/bin/pg_amcheck/pg_amcheck.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..d18d38010d 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -297,6 +297,7 @@ main(int argc, char *argv[])
long_options, &optindex)) != -1)
{
char *endptr;
+ unsigned long optval;
switch (c)
{
@@ -407,30 +408,34 @@ main(int argc, char *argv[])
}
break;
case 7:
- opts.startblock = strtol(optarg, &endptr, 10);
- if (*endptr != '\0')
+ errno = 0;
+ optval = strtoul(optarg, &endptr, 10);
+ if (endptr == optarg || *endptr != '\0' || errno != 0)
{
pg_log_error("invalid start block");
exit(1);
}
- if (opts.startblock > MaxBlockNumber || opts.startblock < 0)
+ if (optval > MaxBlockNumber)
{
pg_log_error("start block out of bounds");
exit(1);
}
+ opts.startblock = optval;
break;
case 8:
- opts.endblock = strtol(optarg, &endptr, 10);
- if (*endptr != '\0')
+ errno = 0;
+ optval = strtoul(optarg, &endptr, 10);
+ if (endptr == optarg || *endptr != '\0' || errno != 0)
{
pg_log_error("invalid end block");
exit(1);
}
- if (opts.endblock > MaxBlockNumber || opts.endblock < 0)
+ if (optval > MaxBlockNumber)
{
pg_log_error("end block out of bounds");
exit(1);
}
+ opts.endblock = optval;
break;
case 9:
opts.rootdescend = true;
--
2.32.0
On Jul 22, 2021, at 7:56 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
Please check that it's up to speed.
<0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch>
This looks correct to me. Thanks for the fix.
Your use of strtoul compares favorably to that in pg_resetwal in that you are checking errno and it is not. The consequence is:
bin % ./pg_resetwal/pg_resetwal -e 1111111111111111111111111111111111111111111111111111
pg_resetwal: error: transaction ID epoch (-e) must not be -1
bin % ./pg_resetwal/pg_resetwal -e junkstring
pg_resetwal: error: invalid argument for option -e
Try "pg_resetwal --help" for more information.
Unless people are relying on this behavior, I would think pg_resetwal should complain of an invalid argument for both of those, rather than complaining about -1. That's not to do with this patch, but if we're tightening up the use of strtol in frontend tools, maybe we can use the identical logic in both places.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jul 22, 2021 at 04:56:56PM +0200, Peter Eisentraut wrote:
It seems to me that when using the pg_amcheck --startblock and --endblock
options on platforms where sizeof(long) == 4, you cannot specify higher
block numbers (unless you do tricks with negative numbers). The attached
patch should fix this by using strtoul() instead of strtol(). I also
tightened up the number scanning a bit in other ways, similar to the code in
other frontend utilities. I know some people have been working on
tightening all this up. Please check that it's up to speed.
Yeah, some work is happening to tighten all that. Saying that, the
first round of review I did for the option parsing is not involving
option types other than int32, so the block options of pg_amcheck are
not changing for now, and what you are suggesting here is fine for the
moment for 14~. Still, note that I am planning to change that on HEAD
with an option parsing API for int64 that could be used for block
numbers, eliminating for example the 4 translatable strings for the
code being changed here.
--
Michael
On 22.07.21 18:18, Mark Dilger wrote:
On Jul 22, 2021, at 7:56 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
Please check that it's up to speed.
<0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch>This looks correct to me. Thanks for the fix.
Committed this to 14 and master.
Your use of strtoul compares favorably to that in pg_resetwal in that you are checking errno and it is not. The consequence is:
bin % ./pg_resetwal/pg_resetwal -e 1111111111111111111111111111111111111111111111111111
pg_resetwal: error: transaction ID epoch (-e) must not be -1
bin % ./pg_resetwal/pg_resetwal -e junkstring
pg_resetwal: error: invalid argument for option -e
Try "pg_resetwal --help" for more information.Unless people are relying on this behavior, I would think pg_resetwal should complain of an invalid argument for both of those, rather than complaining about -1. That's not to do with this patch, but if we're tightening up the use of strtol in frontend tools, maybe we can use the identical logic in both places.
Committed a fix for this to master.