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: https://www.postgresql.org/message-id/20200918095713.ga20...@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
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');
signature.asc
Description: PGP signature
