Skip to content

Commit 4d29e6d

Browse files
michaelpqmichaelpq
committed
Improve range checks of options for pg_test_fsync and pg_test_timing
Both tools never had safeguard checks for the options provided, and it was possible to make pg_test_fsync run an infinite amount of time or pass down buggy values to pg_test_timing. These behaviors have existed for a long time, with no actual complaints, so no backpatch is done. Basic TAP tests are introduced for both tools. Author: Michael Paquier Reviewed-by: Peter Eisentraut Discussion: https://postgr.es/m/20200806062759.GE16470@paquier.xyz
1 parent 41efb83 commit 4d29e6d

File tree

8 files changed

+121
-24
lines changed

8 files changed

+121
-24
lines changed

‎src/bin/pg_test_fsync/.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
11
/pg_test_fsync
2+
3+
/tmp_check/

‎src/bin/pg_test_fsync/Makefile

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ install: all installdirs
2222
installdirs:
2323
$(MKDIR_P) '$(DESTDIR)$(bindir)'
2424

25+
check:
26+
$(prove_check)
27+
28+
installcheck:
29+
$(prove_installcheck)
30+
2531
uninstall:
2632
rm -f '$(DESTDIR)$(bindir)/pg_test_fsync$(X)'
2733

‎src/bin/pg_test_fsync/pg_test_fsync.c

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
#include "postgres_fe.h"
77

8+
#include <limits.h>
89
#include <sys/stat.h>
910
#include <sys/time.h>
1011
#include <fcntl.h>
@@ -62,7 +63,7 @@ do { \
6263

6364
static const char *progname;
6465

65-
static int secs_per_test = 5;
66+
static unsigned int secs_per_test = 5;
6667
static int needs_unlink = 0;
6768
static char full_buf[DEFAULT_XLOG_SEG_SIZE],
6869
*buf,
@@ -148,6 +149,8 @@ handle_args(int argc, char *argv[])
148149

149150
int option; /* Command line option */
150151
int optindex = 0; /* used by getopt_long */
152+
unsigned long optval; /* used for option parsing */
153+
char *endptr;
151154

152155
if (argc > 1)
153156
{
@@ -173,7 +176,24 @@ handle_args(int argc, char *argv[])
173176
break;
174177

175178
case 's':
176-
secs_per_test = atoi(optarg);
179+
errno = 0;
180+
optval = strtoul(optarg, &endptr, 10);
181+
182+
if (endptr == optarg || *endptr != '\0' ||
183+
errno != 0 || optval != (unsigned int) optval)
184+
{
185+
pg_log_error("invalid argument for option %s", "--secs-per-test");
186+
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
187+
exit(1);
188+
}
189+
190+
secs_per_test = (unsigned int) optval;
191+
if (secs_per_test == 0)
192+
{
193+
pg_log_error("%s must be in range %u..%u",
194+
"--secs-per-test", 1, UINT_MAX);
195+
exit(1);
196+
}
177197
break;
178198

179199
default:
@@ -193,8 +213,8 @@ handle_args(int argc, char *argv[])
193213
exit(1);
194214
}
195215

196-
printf(ngettext("%d second per test\n",
197-
"%d seconds per test\n",
216+
printf(ngettext("%u second per test\n",
217+
"%u seconds per test\n",
198218
secs_per_test),
199219
secs_per_test);
200220
#if PG_O_DIRECT != 0

‎src/bin/pg_test_fsync/t/001_basic.pl

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
use strict;
2+
use warnings;
3+
4+
use Config;
5+
use TestLib;
6+
use Test::More tests => 12;
7+
8+
#########################################
9+
# Basic checks
10+
11+
program_help_ok('pg_test_fsync');
12+
program_version_ok('pg_test_fsync');
13+
program_options_handling_ok('pg_test_fsync');
14+
15+
#########################################
16+
# Test invalid option combinations
17+
18+
command_fails_like(
19+
[ 'pg_test_fsync', '--secs-per-test', 'a' ],
20+
qr/\Qpg_test_fsync: error: invalid argument for option --secs-per-test\E/,
21+
'pg_test_fsync: invalid argument for option --secs-per-test');
22+
command_fails_like(
23+
[ 'pg_test_fsync', '--secs-per-test', '0' ],
24+
qr/\Qpg_test_fsync: error: --secs-per-test must be in range 1..4294967295\E/,
25+
'pg_test_fsync: --secs-per-test must be in range');

‎src/bin/pg_test_timing/.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
11
/pg_test_timing
2+
3+
/tmp_check/

‎src/bin/pg_test_timing/Makefile

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ install: all installdirs
2222
installdirs:
2323
$(MKDIR_P) '$(DESTDIR)$(bindir)'
2424

25+
check:
26+
$(prove_check)
27+
28+
installcheck:
29+
$(prove_installcheck)
30+
2531
uninstall:
2632
rm -f '$(DESTDIR)$(bindir)/pg_test_timing$(X)'
2733

‎src/bin/pg_test_timing/pg_test_timing.c

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,17 @@
66

77
#include "postgres_fe.h"
88

9+
#include <limits.h>
10+
911
#include "getopt_long.h"
1012
#include "portability/instr_time.h"
1113

1214
static const char *progname;
1315

14-
static int32 test_duration = 3;
16+
static unsigned int test_duration = 3;
1517

1618
static void handle_args(int argc, char *argv[]);
17-
static uint64 test_timing(int32);
19+
static uint64 test_timing(unsigned int duration);
1820
static void output(uint64 loop_count);
1921

2022
/* record duration in powers of 2 microseconds */
@@ -47,6 +49,8 @@ handle_args(int argc, char *argv[])
4749

4850
int option; /* Command line option */
4951
int optindex = 0; /* used by getopt_long */
52+
unsigned long optval; /* used for option parsing */
53+
char *endptr;
5054

5155
if (argc > 1)
5256
{
@@ -68,7 +72,25 @@ handle_args(int argc, char *argv[])
6872
switch (option)
6973
{
7074
case 'd':
71-
test_duration = atoi(optarg);
75+
errno = 0;
76+
optval = strtoul(optarg, &endptr, 10);
77+
78+
if (endptr == optarg || *endptr != '\0' ||
79+
errno != 0 || optval != (unsigned int) optval)
80+
{
81+
fprintf(stderr, _("%s: invalid argument for option %s\n"),
82+
progname, "--duration");
83+
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
84+
exit(1);
85+
}
86+
87+
test_duration = (unsigned int) optval;
88+
if (test_duration == 0)
89+
{
90+
fprintf(stderr, _("%s: %s must be in range %u..%u\n"),
91+
progname, "--duration", 1, UINT_MAX);
92+
exit(1);
93+
}
7294
break;
7395

7496
default:
@@ -89,26 +111,15 @@ handle_args(int argc, char *argv[])
89111
exit(1);
90112
}
91113

92-
if (test_duration > 0)
93-
{
94-
printf(ngettext("Testing timing overhead for %d second.\n",
95-
"Testing timing overhead for %d seconds.\n",
96-
test_duration),
97-
test_duration);
98-
}
99-
else
100-
{
101-
fprintf(stderr,
102-
_("%s: duration must be a positive integer (duration is \"%d\")\n"),
103-
progname, test_duration);
104-
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
105-
progname);
106-
exit(1);
107-
}
114+
115+
printf(ngettext("Testing timing overhead for %u second.\n",
116+
"Testing timing overhead for %u seconds.\n",
117+
test_duration),
118+
test_duration);
108119
}
109120

110121
static uint64
111-
test_timing(int32 duration)
122+
test_timing(unsigned int duration)
112123
{
113124
uint64 total_time;
114125
int64 time_elapsed = 0;

‎src/bin/pg_test_timing/t/001_basic.pl

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
use strict;
2+
use warnings;
3+
4+
use Config;
5+
use TestLib;
6+
use Test::More tests => 12;
7+
8+
#########################################
9+
# Basic checks
10+
11+
program_help_ok('pg_test_timing');
12+
program_version_ok('pg_test_timing');
13+
program_options_handling_ok('pg_test_timing');
14+
15+
#########################################
16+
# Test invalid option combinations
17+
18+
command_fails_like(
19+
[ 'pg_test_timing', '--duration', 'a' ],
20+
qr/\Qpg_test_timing: invalid argument for option --duration\E/,
21+
'pg_test_timing: invalid argument for option --duration');
22+
command_fails_like(
23+
[ 'pg_test_timing', '--duration', '0' ],
24+
qr/\Qpg_test_timing: --duration must be in range 1..4294967295\E/,
25+
'pg_test_timing: --duration must be in range');

0 commit comments

Comments
 (0)