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
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');
signature.asc
Description: PGP signature
