Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration
Hi all,
As $subject says, pg_test_fsync and pg_test_timing don't really check
the range of option values specified. It is possible for example to
make pg_test_fsync run an infinite amount of time, and pg_test_timing
does not handle overflows with --duration at all.
These are far from being critical issues, but let's fix them at least
on HEAD. So, please see the attached, where I have also added some
basic TAP tests for both tools.
Thanks,
--
Michael
Attachments:
pgtest-fix-range.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_test_fsync/.gitignore b/src/bin/pg_test_fsync/.gitignore
index f3b5932498..5eb5085f45 100644
--- a/src/bin/pg_test_fsync/.gitignore
+++ b/src/bin/pg_test_fsync/.gitignore
@@ -1 +1,3 @@
/pg_test_fsync
+
+/tmp_check/
diff --git a/src/bin/pg_test_fsync/Makefile b/src/bin/pg_test_fsync/Makefile
index 7632c94eb7..c4f9ae0664 100644
--- a/src/bin/pg_test_fsync/Makefile
+++ b/src/bin/pg_test_fsync/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 6e47293123..e56b42bcf8 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -5,6 +5,7 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <fcntl.h>
@@ -174,6 +175,12 @@ handle_args(int argc, char *argv[])
case 's':
secs_per_test = atoi(optarg);
+ if (secs_per_test <= 0 || errno == ERANGE)
+ {
+ pg_log_error("%s must be in range %d..%d",
+ "--secs-per-test", 1, INT_MAX);
+ exit(1);
+ }
break;
default:
diff --git a/src/bin/pg_test_fsync/t/001_basic.pl b/src/bin/pg_test_fsync/t/001_basic.pl
new file mode 100644
index 0000000000..cc5517cb06
--- /dev/null
+++ b/src/bin/pg_test_fsync/t/001_basic.pl
@@ -0,0 +1,21 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 10;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_fsync');
+program_version_ok('pg_test_fsync');
+program_options_handling_ok('pg_test_fsync');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', '0' ],
+ qr/\Qpg_test_fsync: error: --secs-per-test must be in range 1..2147483647\E/,
+ 'pg_test_fsync: --secs-per-test must be in range 1..2147483647');
diff --git a/src/bin/pg_test_timing/.gitignore b/src/bin/pg_test_timing/.gitignore
index f6c664c765..e5aac2ab12 100644
--- a/src/bin/pg_test_timing/.gitignore
+++ b/src/bin/pg_test_timing/.gitignore
@@ -1 +1,3 @@
/pg_test_timing
+
+/tmp_check/
diff --git a/src/bin/pg_test_timing/Makefile b/src/bin/pg_test_timing/Makefile
index 334d6ff5c0..52994b4103 100644
--- a/src/bin/pg_test_timing/Makefile
+++ b/src/bin/pg_test_timing/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index e14802372b..d369d32781 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -6,6 +6,8 @@
#include "postgres_fe.h"
+#include <limits.h>
+
#include "getopt_long.h"
#include "portability/instr_time.h"
@@ -69,6 +71,12 @@ handle_args(int argc, char *argv[])
{
case 'd':
test_duration = atoi(optarg);
+ if (test_duration <= 0 || errno == ERANGE)
+ {
+ fprintf(stderr, _("%s: %s must be in range %d..%d\n"),
+ progname, "--duration", 1, INT_MAX);
+ exit(1);
+ }
break;
default:
@@ -89,22 +97,11 @@ handle_args(int argc, char *argv[])
exit(1);
}
- if (test_duration > 0)
- {
- printf(ngettext("Testing timing overhead for %d second.\n",
- "Testing timing overhead for %d seconds.\n",
- test_duration),
- test_duration);
- }
- else
- {
- fprintf(stderr,
- _("%s: duration must be a positive integer (duration is \"%d\")\n"),
- progname, test_duration);
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
- progname);
- exit(1);
- }
+
+ printf(ngettext("Testing timing overhead for %d second.\n",
+ "Testing timing overhead for %d seconds.\n",
+ test_duration),
+ test_duration);
}
static uint64
diff --git a/src/bin/pg_test_timing/t/001_basic.pl b/src/bin/pg_test_timing/t/001_basic.pl
new file mode 100644
index 0000000000..32dcfbe572
--- /dev/null
+++ b/src/bin/pg_test_timing/t/001_basic.pl
@@ -0,0 +1,21 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 10;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_timing');
+program_version_ok('pg_test_timing');
+program_options_handling_ok('pg_test_timing');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_timing', '--duration', '0' ],
+ qr/\Qpg_test_timing: --duration must be in range 1..2147483647\E/,
+ 'pg_test_timing: --duration must be in range 1..2147483647');
On 2020-08-06 08:27, Michael Paquier wrote:
As $subject says, pg_test_fsync and pg_test_timing don't really check
the range of option values specified. It is possible for example to
make pg_test_fsync run an infinite amount of time, and pg_test_timing
does not handle overflows with --duration at all.These are far from being critical issues, but let's fix them at least
on HEAD. So, please see the attached, where I have also added some
basic TAP tests for both tools.
According to the POSIX standard, atoi() is not required to do any error
checking, and if you want error checking, you should use strtol().
And if you do that, you might as well change the variables to unsigned
and use strtoul(), and then drop the checks for <=0. I would allow 0.
It's not very useful, but it's not harmful and could be applicable in
testing.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Sep 04, 2020 at 11:24:39PM +0200, Peter Eisentraut wrote:
According to the POSIX standard, atoi() is not required to do any error
checking, and if you want error checking, you should use strtol().And if you do that, you might as well change the variables to unsigned and
use strtoul(), and then drop the checks for <=0.
Switching to unsigned makes sense, indeed.
I would allow 0. It's not
very useful, but it's not harmful and could be applicable in testing.
Hmm, OK. For pg_test_fsync, 0 means infinity, and for pg_test_timing
that means stopping immediately (we currently don't allow that). How
does this apply to testing? For pg_test_fsync, using 0 would mean to
just remain stuck in the first fsync() pattern, while for
pg_test_fsync this means doing no test loops at all, generating a
useless log once done. Or do you mean to change the logic of
pg_test_fsync so as --secs-per-test=0 means doing one single write?
That's something I thought about for this thread, but I am not sure
that the extra regression test gain is worth more complexity in this
code.
--
Michael
Attachments:
pgtest-fix-range-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_test_fsync/.gitignore b/src/bin/pg_test_fsync/.gitignore
index f3b5932498..5eb5085f45 100644
--- a/src/bin/pg_test_fsync/.gitignore
+++ b/src/bin/pg_test_fsync/.gitignore
@@ -1 +1,3 @@
/pg_test_fsync
+
+/tmp_check/
diff --git a/src/bin/pg_test_fsync/Makefile b/src/bin/pg_test_fsync/Makefile
index 7632c94eb7..c4f9ae0664 100644
--- a/src/bin/pg_test_fsync/Makefile
+++ b/src/bin/pg_test_fsync/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 6e47293123..db96986684 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -62,7 +62,7 @@ do { \
static const char *progname;
-static int secs_per_test = 5;
+static uint32 secs_per_test = 5;
static int needs_unlink = 0;
static char full_buf[DEFAULT_XLOG_SEG_SIZE],
*buf,
@@ -148,6 +148,7 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ char *endptr;
if (argc > 1)
{
@@ -173,7 +174,19 @@ handle_args(int argc, char *argv[])
break;
case 's':
- secs_per_test = atoi(optarg);
+ secs_per_test = strtoul(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0')
+ {
+ pg_log_error("invalid argument for option %s", "--secs-per-test");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+ if (secs_per_test == 0)
+ {
+ pg_log_error("%s must not be 0", "--secs-per-test");
+ exit(1);
+ }
break;
default:
diff --git a/src/bin/pg_test_fsync/t/001_basic.pl b/src/bin/pg_test_fsync/t/001_basic.pl
new file mode 100644
index 0000000000..3fdb4d07fc
--- /dev/null
+++ b/src/bin/pg_test_fsync/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_fsync');
+program_version_ok('pg_test_fsync');
+program_options_handling_ok('pg_test_fsync');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', 'a' ],
+ qr/\Qpg_test_fsync: error: invalid argument for option --secs-per-test\E/,
+ 'pg_test_fsync: invalid argument for option --secs-per-test');
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', '0' ],
+ qr/\Qpg_test_fsync: error: --secs-per-test must not be 0\E/,
+ 'pg_test_fsync: --secs-per-test must not be 0');
diff --git a/src/bin/pg_test_timing/.gitignore b/src/bin/pg_test_timing/.gitignore
index f6c664c765..e5aac2ab12 100644
--- a/src/bin/pg_test_timing/.gitignore
+++ b/src/bin/pg_test_timing/.gitignore
@@ -1 +1,3 @@
/pg_test_timing
+
+/tmp_check/
diff --git a/src/bin/pg_test_timing/Makefile b/src/bin/pg_test_timing/Makefile
index 334d6ff5c0..52994b4103 100644
--- a/src/bin/pg_test_timing/Makefile
+++ b/src/bin/pg_test_timing/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index e14802372b..229c2f27b6 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -11,10 +11,10 @@
static const char *progname;
-static int32 test_duration = 3;
+static uint32 test_duration = 3;
static void handle_args(int argc, char *argv[]);
-static uint64 test_timing(int32);
+static uint64 test_timing(uint32 duration);
static void output(uint64 loop_count);
/* record duration in powers of 2 microseconds */
@@ -47,6 +47,7 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ char *endptr;
if (argc > 1)
{
@@ -68,7 +69,21 @@ handle_args(int argc, char *argv[])
switch (option)
{
case 'd':
- test_duration = atoi(optarg);
+ test_duration = strtoul(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0')
+ {
+ fprintf(stderr, _("%s: invalid argument for option %s\n"),
+ progname, "--duration");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+ if (test_duration == 0)
+ {
+ fprintf(stderr, _("%s: %s must not be 0\n"), progname,
+ "--duration");
+ exit(1);
+ }
break;
default:
@@ -89,26 +104,15 @@ handle_args(int argc, char *argv[])
exit(1);
}
- if (test_duration > 0)
- {
- printf(ngettext("Testing timing overhead for %d second.\n",
- "Testing timing overhead for %d seconds.\n",
- test_duration),
- test_duration);
- }
- else
- {
- fprintf(stderr,
- _("%s: duration must be a positive integer (duration is \"%d\")\n"),
- progname, test_duration);
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
- progname);
- exit(1);
- }
+
+ printf(ngettext("Testing timing overhead for %d second.\n",
+ "Testing timing overhead for %d seconds.\n",
+ test_duration),
+ test_duration);
}
static uint64
-test_timing(int32 duration)
+test_timing(uint32 duration)
{
uint64 total_time;
int64 time_elapsed = 0;
diff --git a/src/bin/pg_test_timing/t/001_basic.pl b/src/bin/pg_test_timing/t/001_basic.pl
new file mode 100644
index 0000000000..d194ad04ea
--- /dev/null
+++ b/src/bin/pg_test_timing/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_timing');
+program_version_ok('pg_test_timing');
+program_options_handling_ok('pg_test_timing');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_timing', '--duration', 'a' ],
+ qr/\Qpg_test_timing: invalid argument for option --duration\E/,
+ 'pg_test_timing: invalid argument for option --duration');
+command_fails_like(
+ [ 'pg_test_timing', '--duration', '0' ],
+ qr/\Qpg_test_timing: --duration must not be 0\E/,
+ 'pg_test_timing: --duration must not be 0');
On 2020-09-06 05:04, Michael Paquier wrote:
I would allow 0. It's not
very useful, but it's not harmful and could be applicable in testing.Hmm, OK. For pg_test_fsync, 0 means infinity, and for pg_test_timing
that means stopping immediately (we currently don't allow that). How
does this apply to testing? For pg_test_fsync, using 0 would mean to
just remain stuck in the first fsync() pattern, while for
pg_test_fsync this means doing no test loops at all, generating a
useless log once done. Or do you mean to change the logic of
pg_test_fsync so as --secs-per-test=0 means doing one single write?
That's something I thought about for this thread, but I am not sure
that the extra regression test gain is worth more complexity in this
code.
I think in general doing something 0 times should be allowed if possible.
However, I see that in the case of pg_test_fsync you end up in alarm(0),
which does something different, so it's okay in that case to disallow it.
I notice that the error checking you introduce is different from the
checks for pgbench -t and -T (the latter having no errno checks). I'm
not sure which is correct, but it's perhaps worth making them the same.
(pgbench -t 0, which is also currently not allowed, is a good example of
why this could be useful, because that would allow checking whether the
script etc. can be loaded without running an actual test.)
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Sep 07, 2020 at 10:06:57AM +0200, Peter Eisentraut wrote:
However, I see that in the case of pg_test_fsync you end up in alarm(0),
which does something different, so it's okay in that case to disallow it.
Yep.
I notice that the error checking you introduce is different from the checks
for pgbench -t and -T (the latter having no errno checks). I'm not sure
which is correct, but it's perhaps worth making them the same.
pgbench currently uses atoi() to parse the options of -t and -T. Are
you suggesting to switch that to strtoXX() as well or perhaps you are
referring to the parsing of the weight in parseScriptWeight()? FWIW,
the error handling introduced in this patch is similar to what we do
for example in pg_resetwal. This has its own problems as strtoul()
would not report ERANGE except for values higher than ULONG_MAX, but
the returned results are stored in 32 bits. We could switch to just
use uint64 where we could of course, but is that really worth it for
such tools? For example, pg_test_timing could overflow the
total_timing calculated if using a too high value, but nobody would
use such values anyway. So I'd rather just use uint32 and call it a
day, for simplicity's sake mainly..
(pgbench -t 0, which is also currently not allowed, is a good example of why
this could be useful, because that would allow checking whether the script
etc. can be loaded without running an actual test.)
Perhaps. That looks like a separate item to me though.
--
Michael
On 2020-09-10 09:59, Michael Paquier wrote:
I notice that the error checking you introduce is different from the checks
for pgbench -t and -T (the latter having no errno checks). I'm not sure
which is correct, but it's perhaps worth making them the same.pgbench currently uses atoi() to parse the options of -t and -T. Are
you suggesting to switch that to strtoXX() as well or perhaps you are
referring to the parsing of the weight in parseScriptWeight()? FWIW,
the error handling introduced in this patch is similar to what we do
for example in pg_resetwal. This has its own problems as strtoul()
would not report ERANGE except for values higher than ULONG_MAX, but
the returned results are stored in 32 bits. We could switch to just
use uint64 where we could of course, but is that really worth it for
such tools? For example, pg_test_timing could overflow the
total_timing calculated if using a too high value, but nobody would
use such values anyway. So I'd rather just use uint32 and call it a
day, for simplicity's sake mainly..
The first patch you proposed checks for errno == ERANGE, but pgbench
code doesn't do that. So one of them is not correct.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 10, 2020 at 03:59:20PM +0200, Peter Eisentraut wrote:
The first patch you proposed checks for errno == ERANGE, but pgbench code
doesn't do that. So one of them is not correct.
Sorry for the confusion, I misunderstood what you were referring to.
Yes, the first patch is wrong to add the check on errno. FWIW, I
thought about your point to use strtol() but that does not seem worth
the complication for those tools. It is not like anybody is going to
use high values for these, and using uint64 to make sure that the
boundaries are checked just add more checks for bounds. There is
one example in pg_test_timing when compiling the total time.
--
Michael
On 2020-09-11 09:08, Michael Paquier wrote:
On Thu, Sep 10, 2020 at 03:59:20PM +0200, Peter Eisentraut wrote:
The first patch you proposed checks for errno == ERANGE, but pgbench code
doesn't do that. So one of them is not correct.Sorry for the confusion, I misunderstood what you were referring to.
Yes, the first patch is wrong to add the check on errno. FWIW, I
thought about your point to use strtol() but that does not seem worth
the complication for those tools. It is not like anybody is going to
use high values for these, and using uint64 to make sure that the
boundaries are checked just add more checks for bounds. There is
one example in pg_test_timing when compiling the total time.
I didn't mean use strtol() to be able to process larger values, but for
the error checking. atoi() cannot detect any errors other than ERANGE.
So if you are spending effort on making the option value parsing more
robust, relying on atoi() will result in an incomplete solution.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 15, 2020 at 02:39:08PM +0200, Peter Eisentraut wrote:
I didn't mean use strtol() to be able to process larger values, but for the
error checking. atoi() cannot detect any errors other than ERANGE. So if
you are spending effort on making the option value parsing more robust,
relying on atoi() will result in an incomplete solution.
Okay, after looking at that, here is v3. This includes range checks
as well as errno checks based on strtol(). What do you think?
--
Michael
Attachments:
pgtest-fix-range-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_test_fsync/.gitignore b/src/bin/pg_test_fsync/.gitignore
index f3b5932498..5eb5085f45 100644
--- a/src/bin/pg_test_fsync/.gitignore
+++ b/src/bin/pg_test_fsync/.gitignore
@@ -1 +1,3 @@
/pg_test_fsync
+
+/tmp_check/
diff --git a/src/bin/pg_test_fsync/Makefile b/src/bin/pg_test_fsync/Makefile
index 7632c94eb7..c4f9ae0664 100644
--- a/src/bin/pg_test_fsync/Makefile
+++ b/src/bin/pg_test_fsync/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 6e47293123..3ab78ac0d5 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -5,6 +5,7 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <fcntl.h>
@@ -62,7 +63,7 @@ do { \
static const char *progname;
-static int secs_per_test = 5;
+static int32 secs_per_test = 5;
static int needs_unlink = 0;
static char full_buf[DEFAULT_XLOG_SEG_SIZE],
*buf,
@@ -148,6 +149,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -173,7 +176,23 @@ handle_args(int argc, char *argv[])
break;
case 's':
- secs_per_test = atoi(optarg);
+ optval = strtol(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (int32) optval)
+ {
+ pg_log_error("invalid argument for option %s", "--secs-per-test");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ secs_per_test = (int32) optval;
+ if (secs_per_test <= 0)
+ {
+ pg_log_error("%s must be in range %d..%d",
+ "--secs-per-test", 1, INT_MAX);
+ exit(1);
+ }
break;
default:
diff --git a/src/bin/pg_test_fsync/t/001_basic.pl b/src/bin/pg_test_fsync/t/001_basic.pl
new file mode 100644
index 0000000000..d4b67092cb
--- /dev/null
+++ b/src/bin/pg_test_fsync/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_fsync');
+program_version_ok('pg_test_fsync');
+program_options_handling_ok('pg_test_fsync');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', 'a' ],
+ qr/\Qpg_test_fsync: error: invalid argument for option --secs-per-test\E/,
+ 'pg_test_fsync: invalid argument for option --secs-per-test');
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', '0' ],
+ qr/\Qpg_test_fsync: error: --secs-per-test must be in range 1..2147483647\E/,
+ 'pg_test_fsync: --secs-per-test must be in range');
diff --git a/src/bin/pg_test_timing/.gitignore b/src/bin/pg_test_timing/.gitignore
index f6c664c765..e5aac2ab12 100644
--- a/src/bin/pg_test_timing/.gitignore
+++ b/src/bin/pg_test_timing/.gitignore
@@ -1 +1,3 @@
/pg_test_timing
+
+/tmp_check/
diff --git a/src/bin/pg_test_timing/Makefile b/src/bin/pg_test_timing/Makefile
index 334d6ff5c0..52994b4103 100644
--- a/src/bin/pg_test_timing/Makefile
+++ b/src/bin/pg_test_timing/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index e14802372b..2a19ac6368 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -6,6 +6,8 @@
#include "postgres_fe.h"
+#include <limits.h>
+
#include "getopt_long.h"
#include "portability/instr_time.h"
@@ -14,7 +16,7 @@ static const char *progname;
static int32 test_duration = 3;
static void handle_args(int argc, char *argv[]);
-static uint64 test_timing(int32);
+static uint64 test_timing(int32 duration);
static void output(uint64 loop_count);
/* record duration in powers of 2 microseconds */
@@ -47,6 +49,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -68,7 +72,24 @@ handle_args(int argc, char *argv[])
switch (option)
{
case 'd':
- test_duration = atoi(optarg);
+ optval = strtol(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (int32) optval)
+ {
+ fprintf(stderr, _("%s: invalid argument for option %s\n"),
+ progname, "--duration");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ test_duration = (int32) optval;
+ if (test_duration <= 0)
+ {
+ fprintf(stderr, _("%s: %s must be in range %d..%d\n"),
+ progname, "--duration", 1, INT_MAX);
+ exit(1);
+ }
break;
default:
@@ -89,22 +110,11 @@ handle_args(int argc, char *argv[])
exit(1);
}
- if (test_duration > 0)
- {
- printf(ngettext("Testing timing overhead for %d second.\n",
- "Testing timing overhead for %d seconds.\n",
- test_duration),
- test_duration);
- }
- else
- {
- fprintf(stderr,
- _("%s: duration must be a positive integer (duration is \"%d\")\n"),
- progname, test_duration);
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
- progname);
- exit(1);
- }
+
+ printf(ngettext("Testing timing overhead for %d second.\n",
+ "Testing timing overhead for %d seconds.\n",
+ test_duration),
+ test_duration);
}
static uint64
diff --git a/src/bin/pg_test_timing/t/001_basic.pl b/src/bin/pg_test_timing/t/001_basic.pl
new file mode 100644
index 0000000000..303725537b
--- /dev/null
+++ b/src/bin/pg_test_timing/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_timing');
+program_version_ok('pg_test_timing');
+program_options_handling_ok('pg_test_timing');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_timing', '--duration', 'a' ],
+ qr/\Qpg_test_timing: invalid argument for option --duration\E/,
+ 'pg_test_timing: invalid argument for option --duration');
+command_fails_like(
+ [ 'pg_test_timing', '--duration', '0' ],
+ qr/\Qpg_test_timing: --duration must be in range 1..2147483647\E/,
+ 'pg_test_timing: --duration must be in range');
On Fri, Sep 18, 2020 at 05:22:15PM +0900, Michael Paquier wrote:
Okay, after looking at that, here is v3. This includes range checks
as well as errno checks based on strtol(). What do you think?
This fails in the CF bot on Linux because of pg_logging_init()
returning with errno=ENOTTY in the TAP tests, for which I began a new
thread:
/messages/by-id/20200918095713.GA20887@paquier.xyz
Not sure if this will lead anywhere, but we can also address the
failure by enforcing errno=0 for the new calls of strtol() introduced
in this patch. So here is an updated patch doing so.
--
Michael
Attachments:
pgtest-fix-range-v4.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_test_fsync/.gitignore b/src/bin/pg_test_fsync/.gitignore
index f3b5932498..5eb5085f45 100644
--- a/src/bin/pg_test_fsync/.gitignore
+++ b/src/bin/pg_test_fsync/.gitignore
@@ -1 +1,3 @@
/pg_test_fsync
+
+/tmp_check/
diff --git a/src/bin/pg_test_fsync/Makefile b/src/bin/pg_test_fsync/Makefile
index 7632c94eb7..c4f9ae0664 100644
--- a/src/bin/pg_test_fsync/Makefile
+++ b/src/bin/pg_test_fsync/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 6e47293123..1a09c36960 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -5,6 +5,7 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <fcntl.h>
@@ -62,7 +63,7 @@ do { \
static const char *progname;
-static int secs_per_test = 5;
+static int32 secs_per_test = 5;
static int needs_unlink = 0;
static char full_buf[DEFAULT_XLOG_SEG_SIZE],
*buf,
@@ -148,6 +149,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -173,7 +176,24 @@ handle_args(int argc, char *argv[])
break;
case 's':
- secs_per_test = atoi(optarg);
+ errno = 0;
+ optval = strtol(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (int32) optval)
+ {
+ pg_log_error("invalid argument for option %s", "--secs-per-test");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ secs_per_test = (int32) optval;
+ if (secs_per_test <= 0)
+ {
+ pg_log_error("%s must be in range %d..%d",
+ "--secs-per-test", 1, INT_MAX);
+ exit(1);
+ }
break;
default:
diff --git a/src/bin/pg_test_fsync/t/001_basic.pl b/src/bin/pg_test_fsync/t/001_basic.pl
new file mode 100644
index 0000000000..4b615c263d
--- /dev/null
+++ b/src/bin/pg_test_fsync/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_fsync');
+program_version_ok('pg_test_fsync');
+program_options_handling_ok('pg_test_fsync');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', 'a' ],
+ qr/\Qpg_test_fsync: error: invalid argument for option --secs-per-test\E/,
+ 'pg_test_fsync: invalid argument for option --secs-per-test');
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', '0' ],
+ qr/\Qpg_test_fsync: error: --secs-per-test must be in range 1..2147483647\E/,
+ 'pg_test_fsync: --secs-per-test must be in range');
diff --git a/src/bin/pg_test_timing/.gitignore b/src/bin/pg_test_timing/.gitignore
index f6c664c765..e5aac2ab12 100644
--- a/src/bin/pg_test_timing/.gitignore
+++ b/src/bin/pg_test_timing/.gitignore
@@ -1 +1,3 @@
/pg_test_timing
+
+/tmp_check/
diff --git a/src/bin/pg_test_timing/Makefile b/src/bin/pg_test_timing/Makefile
index 334d6ff5c0..52994b4103 100644
--- a/src/bin/pg_test_timing/Makefile
+++ b/src/bin/pg_test_timing/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index e14802372b..cd16fc2f83 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -6,6 +6,8 @@
#include "postgres_fe.h"
+#include <limits.h>
+
#include "getopt_long.h"
#include "portability/instr_time.h"
@@ -14,7 +16,7 @@ static const char *progname;
static int32 test_duration = 3;
static void handle_args(int argc, char *argv[]);
-static uint64 test_timing(int32);
+static uint64 test_timing(int32 duration);
static void output(uint64 loop_count);
/* record duration in powers of 2 microseconds */
@@ -47,6 +49,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -68,7 +72,25 @@ handle_args(int argc, char *argv[])
switch (option)
{
case 'd':
- test_duration = atoi(optarg);
+ errno = 0;
+ optval = strtol(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (int32) optval)
+ {
+ fprintf(stderr, _("%s: invalid argument for option %s\n"),
+ progname, "--duration");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ test_duration = (int32) optval;
+ if (test_duration <= 0)
+ {
+ fprintf(stderr, _("%s: %s must be in range %d..%d\n"),
+ progname, "--duration", 1, INT_MAX);
+ exit(1);
+ }
break;
default:
@@ -89,22 +111,11 @@ handle_args(int argc, char *argv[])
exit(1);
}
- if (test_duration > 0)
- {
- printf(ngettext("Testing timing overhead for %d second.\n",
- "Testing timing overhead for %d seconds.\n",
- test_duration),
- test_duration);
- }
- else
- {
- fprintf(stderr,
- _("%s: duration must be a positive integer (duration is \"%d\")\n"),
- progname, test_duration);
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
- progname);
- exit(1);
- }
+
+ printf(ngettext("Testing timing overhead for %d second.\n",
+ "Testing timing overhead for %d seconds.\n",
+ test_duration),
+ test_duration);
}
static uint64
diff --git a/src/bin/pg_test_timing/t/001_basic.pl b/src/bin/pg_test_timing/t/001_basic.pl
new file mode 100644
index 0000000000..303725537b
--- /dev/null
+++ b/src/bin/pg_test_timing/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_timing');
+program_version_ok('pg_test_timing');
+program_options_handling_ok('pg_test_timing');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_timing', '--duration', 'a' ],
+ qr/\Qpg_test_timing: invalid argument for option --duration\E/,
+ 'pg_test_timing: invalid argument for option --duration');
+command_fails_like(
+ [ 'pg_test_timing', '--duration', '0' ],
+ qr/\Qpg_test_timing: --duration must be in range 1..2147483647\E/,
+ 'pg_test_timing: --duration must be in range');
On 2020-09-20 05:41, Michael Paquier wrote:
On Fri, Sep 18, 2020 at 05:22:15PM +0900, Michael Paquier wrote:
Okay, after looking at that, here is v3. This includes range checks
as well as errno checks based on strtol(). What do you think?This fails in the CF bot on Linux because of pg_logging_init()
returning with errno=ENOTTY in the TAP tests, for which I began a new
thread:
/messages/by-id/20200918095713.GA20887@paquier.xyzNot sure if this will lead anywhere, but we can also address the
failure by enforcing errno=0 for the new calls of strtol() introduced
in this patch. So here is an updated patch doing so.
I think the error checking is now structurally correct in this patch.
However, I still think the integer type use is a bit inconsistent. In
both cases, using strtoul() and dealing with unsigned integer types
between parsing and final use would be more consistent.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Sep 22, 2020 at 11:45:14PM +0200, Peter Eisentraut wrote:
However, I still think the integer type use is a bit inconsistent. In both
cases, using strtoul() and dealing with unsigned integer types between
parsing and final use would be more consistent.
No objections to that either, so changed this way. I kept those
variables signed because applying values of 2B~4B is not really going
to matter much here ;p
--
Michael
Attachments:
pgtest-fix-range-v5.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_test_fsync/.gitignore b/src/bin/pg_test_fsync/.gitignore
index f3b5932498..5eb5085f45 100644
--- a/src/bin/pg_test_fsync/.gitignore
+++ b/src/bin/pg_test_fsync/.gitignore
@@ -1 +1,3 @@
/pg_test_fsync
+
+/tmp_check/
diff --git a/src/bin/pg_test_fsync/Makefile b/src/bin/pg_test_fsync/Makefile
index 7632c94eb7..c4f9ae0664 100644
--- a/src/bin/pg_test_fsync/Makefile
+++ b/src/bin/pg_test_fsync/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 6e47293123..7119ed0512 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -5,6 +5,7 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <fcntl.h>
@@ -62,7 +63,7 @@ do { \
static const char *progname;
-static int secs_per_test = 5;
+static uint32 secs_per_test = 5;
static int needs_unlink = 0;
static char full_buf[DEFAULT_XLOG_SEG_SIZE],
*buf,
@@ -148,6 +149,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ unsigned long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -173,7 +176,24 @@ handle_args(int argc, char *argv[])
break;
case 's':
- secs_per_test = atoi(optarg);
+ errno = 0;
+ optval = strtoul(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (uint32) optval)
+ {
+ pg_log_error("invalid argument for option %s", "--secs-per-test");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ secs_per_test = (uint32) optval;
+ if (secs_per_test == 0)
+ {
+ pg_log_error("%s must be in range %u..%u",
+ "--secs-per-test", 1, UINT_MAX);
+ exit(1);
+ }
break;
default:
@@ -193,8 +213,8 @@ handle_args(int argc, char *argv[])
exit(1);
}
- printf(ngettext("%d second per test\n",
- "%d seconds per test\n",
+ printf(ngettext("%u second per test\n",
+ "%u seconds per test\n",
secs_per_test),
secs_per_test);
#if PG_O_DIRECT != 0
diff --git a/src/bin/pg_test_fsync/t/001_basic.pl b/src/bin/pg_test_fsync/t/001_basic.pl
new file mode 100644
index 0000000000..fe9c295c49
--- /dev/null
+++ b/src/bin/pg_test_fsync/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_fsync');
+program_version_ok('pg_test_fsync');
+program_options_handling_ok('pg_test_fsync');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', 'a' ],
+ qr/\Qpg_test_fsync: error: invalid argument for option --secs-per-test\E/,
+ 'pg_test_fsync: invalid argument for option --secs-per-test');
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', '0' ],
+ qr/\Qpg_test_fsync: error: --secs-per-test must be in range 1..4294967295\E/,
+ 'pg_test_fsync: --secs-per-test must be in range');
diff --git a/src/bin/pg_test_timing/.gitignore b/src/bin/pg_test_timing/.gitignore
index f6c664c765..e5aac2ab12 100644
--- a/src/bin/pg_test_timing/.gitignore
+++ b/src/bin/pg_test_timing/.gitignore
@@ -1 +1,3 @@
/pg_test_timing
+
+/tmp_check/
diff --git a/src/bin/pg_test_timing/Makefile b/src/bin/pg_test_timing/Makefile
index 334d6ff5c0..52994b4103 100644
--- a/src/bin/pg_test_timing/Makefile
+++ b/src/bin/pg_test_timing/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index e14802372b..c878b19035 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -6,15 +6,17 @@
#include "postgres_fe.h"
+#include <limits.h>
+
#include "getopt_long.h"
#include "portability/instr_time.h"
static const char *progname;
-static int32 test_duration = 3;
+static uint32 test_duration = 3;
static void handle_args(int argc, char *argv[]);
-static uint64 test_timing(int32);
+static uint64 test_timing(uint32 duration);
static void output(uint64 loop_count);
/* record duration in powers of 2 microseconds */
@@ -47,6 +49,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ unsigned long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -68,7 +72,25 @@ handle_args(int argc, char *argv[])
switch (option)
{
case 'd':
- test_duration = atoi(optarg);
+ errno = 0;
+ optval = strtoul(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (uint32) optval)
+ {
+ fprintf(stderr, _("%s: invalid argument for option %s\n"),
+ progname, "--duration");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ test_duration = (uint32) optval;
+ if (test_duration == 0)
+ {
+ fprintf(stderr, _("%s: %s must be in range %u..%u\n"),
+ progname, "--duration", 1, UINT_MAX);
+ exit(1);
+ }
break;
default:
@@ -89,26 +111,15 @@ handle_args(int argc, char *argv[])
exit(1);
}
- if (test_duration > 0)
- {
- printf(ngettext("Testing timing overhead for %d second.\n",
- "Testing timing overhead for %d seconds.\n",
- test_duration),
- test_duration);
- }
- else
- {
- fprintf(stderr,
- _("%s: duration must be a positive integer (duration is \"%d\")\n"),
- progname, test_duration);
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
- progname);
- exit(1);
- }
+
+ printf(ngettext("Testing timing overhead for %u second.\n",
+ "Testing timing overhead for %u seconds.\n",
+ test_duration),
+ test_duration);
}
static uint64
-test_timing(int32 duration)
+test_timing(uint32 duration)
{
uint64 total_time;
int64 time_elapsed = 0;
diff --git a/src/bin/pg_test_timing/t/001_basic.pl b/src/bin/pg_test_timing/t/001_basic.pl
new file mode 100644
index 0000000000..8bad19c7fa
--- /dev/null
+++ b/src/bin/pg_test_timing/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_timing');
+program_version_ok('pg_test_timing');
+program_options_handling_ok('pg_test_timing');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_timing', '--duration', 'a' ],
+ qr/\Qpg_test_timing: invalid argument for option --duration\E/,
+ 'pg_test_timing: invalid argument for option --duration');
+command_fails_like(
+ [ 'pg_test_timing', '--duration', '0' ],
+ qr/\Qpg_test_timing: --duration must be in range 1..4294967295\E/,
+ 'pg_test_timing: --duration must be in range');
On 2020-09-23 03:50, Michael Paquier wrote:
On Tue, Sep 22, 2020 at 11:45:14PM +0200, Peter Eisentraut wrote:
However, I still think the integer type use is a bit inconsistent. In both
cases, using strtoul() and dealing with unsigned integer types between
parsing and final use would be more consistent.No objections to that either, so changed this way. I kept those
variables signed because applying values of 2B~4B is not really going
to matter much here ;p
This patch mixes up unsigned int and uint32 in random ways. The
variable is uint32, but the format is %u and the max constant is UINT_MAX.
I think just use unsigned int as the variable type. There is no need to
use the bit-exact types. Note that the argument of alarm() is of type
unsigned int.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 23, 2020 at 08:11:59AM +0200, Peter Eisentraut wrote:
This patch mixes up unsigned int and uint32 in random ways. The variable is
uint32, but the format is %u and the max constant is UINT_MAX.I think just use unsigned int as the variable type. There is no need to use
the bit-exact types. Note that the argument of alarm() is of type unsigned
int.
Makes sense, thanks.
--
Michael
Attachments:
pgtest-fix-range-v6.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_test_fsync/.gitignore b/src/bin/pg_test_fsync/.gitignore
index f3b5932498..5eb5085f45 100644
--- a/src/bin/pg_test_fsync/.gitignore
+++ b/src/bin/pg_test_fsync/.gitignore
@@ -1 +1,3 @@
/pg_test_fsync
+
+/tmp_check/
diff --git a/src/bin/pg_test_fsync/Makefile b/src/bin/pg_test_fsync/Makefile
index 7632c94eb7..c4f9ae0664 100644
--- a/src/bin/pg_test_fsync/Makefile
+++ b/src/bin/pg_test_fsync/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 6e47293123..c66492eba4 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -5,6 +5,7 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <fcntl.h>
@@ -62,7 +63,7 @@ do { \
static const char *progname;
-static int secs_per_test = 5;
+static unsigned int secs_per_test = 5;
static int needs_unlink = 0;
static char full_buf[DEFAULT_XLOG_SEG_SIZE],
*buf,
@@ -148,6 +149,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ unsigned long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -173,7 +176,24 @@ handle_args(int argc, char *argv[])
break;
case 's':
- secs_per_test = atoi(optarg);
+ errno = 0;
+ optval = strtoul(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (unsigned int) optval)
+ {
+ pg_log_error("invalid argument for option %s", "--secs-per-test");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ secs_per_test = (unsigned int) optval;
+ if (secs_per_test == 0)
+ {
+ pg_log_error("%s must be in range %u..%u",
+ "--secs-per-test", 1, UINT_MAX);
+ exit(1);
+ }
break;
default:
@@ -193,8 +213,8 @@ handle_args(int argc, char *argv[])
exit(1);
}
- printf(ngettext("%d second per test\n",
- "%d seconds per test\n",
+ printf(ngettext("%u second per test\n",
+ "%u seconds per test\n",
secs_per_test),
secs_per_test);
#if PG_O_DIRECT != 0
diff --git a/src/bin/pg_test_fsync/t/001_basic.pl b/src/bin/pg_test_fsync/t/001_basic.pl
new file mode 100644
index 0000000000..fe9c295c49
--- /dev/null
+++ b/src/bin/pg_test_fsync/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_fsync');
+program_version_ok('pg_test_fsync');
+program_options_handling_ok('pg_test_fsync');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', 'a' ],
+ qr/\Qpg_test_fsync: error: invalid argument for option --secs-per-test\E/,
+ 'pg_test_fsync: invalid argument for option --secs-per-test');
+command_fails_like(
+ [ 'pg_test_fsync', '--secs-per-test', '0' ],
+ qr/\Qpg_test_fsync: error: --secs-per-test must be in range 1..4294967295\E/,
+ 'pg_test_fsync: --secs-per-test must be in range');
diff --git a/src/bin/pg_test_timing/.gitignore b/src/bin/pg_test_timing/.gitignore
index f6c664c765..e5aac2ab12 100644
--- a/src/bin/pg_test_timing/.gitignore
+++ b/src/bin/pg_test_timing/.gitignore
@@ -1 +1,3 @@
/pg_test_timing
+
+/tmp_check/
diff --git a/src/bin/pg_test_timing/Makefile b/src/bin/pg_test_timing/Makefile
index 334d6ff5c0..52994b4103 100644
--- a/src/bin/pg_test_timing/Makefile
+++ b/src/bin/pg_test_timing/Makefile
@@ -22,6 +22,12 @@ install: all installdirs
installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
uninstall:
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index e14802372b..894bb1d0d4 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -6,15 +6,17 @@
#include "postgres_fe.h"
+#include <limits.h>
+
#include "getopt_long.h"
#include "portability/instr_time.h"
static const char *progname;
-static int32 test_duration = 3;
+static unsigned int test_duration = 3;
static void handle_args(int argc, char *argv[]);
-static uint64 test_timing(int32);
+static uint64 test_timing(unsigned int duration);
static void output(uint64 loop_count);
/* record duration in powers of 2 microseconds */
@@ -47,6 +49,8 @@ handle_args(int argc, char *argv[])
int option; /* Command line option */
int optindex = 0; /* used by getopt_long */
+ unsigned long optval; /* used for option parsing */
+ char *endptr;
if (argc > 1)
{
@@ -68,7 +72,25 @@ handle_args(int argc, char *argv[])
switch (option)
{
case 'd':
- test_duration = atoi(optarg);
+ errno = 0;
+ optval = strtoul(optarg, &endptr, 10);
+
+ if (endptr == optarg || *endptr != '\0' ||
+ errno != 0 || optval != (unsigned int) optval)
+ {
+ fprintf(stderr, _("%s: invalid argument for option %s\n"),
+ progname, "--duration");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ test_duration = (unsigned int) optval;
+ if (test_duration == 0)
+ {
+ fprintf(stderr, _("%s: %s must be in range %u..%u\n"),
+ progname, "--duration", 1, UINT_MAX);
+ exit(1);
+ }
break;
default:
@@ -89,26 +111,15 @@ handle_args(int argc, char *argv[])
exit(1);
}
- if (test_duration > 0)
- {
- printf(ngettext("Testing timing overhead for %d second.\n",
- "Testing timing overhead for %d seconds.\n",
- test_duration),
- test_duration);
- }
- else
- {
- fprintf(stderr,
- _("%s: duration must be a positive integer (duration is \"%d\")\n"),
- progname, test_duration);
- fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
- progname);
- exit(1);
- }
+
+ printf(ngettext("Testing timing overhead for %u second.\n",
+ "Testing timing overhead for %u seconds.\n",
+ test_duration),
+ test_duration);
}
static uint64
-test_timing(int32 duration)
+test_timing(unsigned int duration)
{
uint64 total_time;
int64 time_elapsed = 0;
diff --git a/src/bin/pg_test_timing/t/001_basic.pl b/src/bin/pg_test_timing/t/001_basic.pl
new file mode 100644
index 0000000000..8bad19c7fa
--- /dev/null
+++ b/src/bin/pg_test_timing/t/001_basic.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use Config;
+use TestLib;
+use Test::More tests => 12;
+
+#########################################
+# Basic checks
+
+program_help_ok('pg_test_timing');
+program_version_ok('pg_test_timing');
+program_options_handling_ok('pg_test_timing');
+
+#########################################
+# Test invalid option combinations
+
+command_fails_like(
+ [ 'pg_test_timing', '--duration', 'a' ],
+ qr/\Qpg_test_timing: invalid argument for option --duration\E/,
+ 'pg_test_timing: invalid argument for option --duration');
+command_fails_like(
+ [ 'pg_test_timing', '--duration', '0' ],
+ qr/\Qpg_test_timing: --duration must be in range 1..4294967295\E/,
+ 'pg_test_timing: --duration must be in range');
On 2020-09-24 09:12, Michael Paquier wrote:
On Wed, Sep 23, 2020 at 08:11:59AM +0200, Peter Eisentraut wrote:
This patch mixes up unsigned int and uint32 in random ways. The variable is
uint32, but the format is %u and the max constant is UINT_MAX.I think just use unsigned int as the variable type. There is no need to use
the bit-exact types. Note that the argument of alarm() is of type unsigned
int.Makes sense, thanks.
looks good to me
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services