Thread: Incorrect usage of strtol, atoi for non-numeric junk inputs
Hi,
While working on [1], I found that some parts of the code is using
strtol and atoi without checking for non-numeric junk input strings. I
found this strange. Most of the time users provide proper numeric
strings but there can be some scenarios where these strings are not
user-supplied but generated by some other code which may contain
non-numeric strings. Shouldn't the code use strtol or atoi
appropriately and error out in such cases? One way to fix this once
and for all is to have a common API something like int
pg_strtol/pg_str_convert_to_int(char *opt_name, char *opt_value) which
returns a generic message upon invalid strings ("invalid value \"%s\"
is provided for option \"%s\", opt_name, opt_value) and returns
integers on successful parsing.
Although this is not a critical issue, I would like to seek thoughts.
[1] - https://www.postgresql.org/message-id/CALj2ACVMO6wY5Pc4oe1OCgUOAtdjHuFsBDw8R5uoYR86eWFQDA%40mail.gmail.com
[2] strtol:
vacuumlo.c --> ./vacuumlo -l '2323adfd' postgres -p '5432ERE'
libpq_pipeline.c --> ./libpq_pipeline -r '2232adf' tests
atoi:
pg_amcheck.c --> ./pg_amcheck -j '1211efe'
pg_basebackup --> ./pg_basebackup -Z 'EFEF' -s 'a$##'
pg_receivewal.c --> ./pg_receivewal -p '54343GDFD' -s '54343GDFD'
pg_recvlogical.c --> ./pg_recvlogical -F '-$#$#' -p '5432$$$' -s '100$$$'
pg_checksums.c. --> ./pg_checksums -f '1212abc'
pg_ctl.c --> ./pg_ctl -t 'efc'
pg_dump.c --> ./pg_dump -j '454adc' -Z '4adc' --extra-float-digits '-14adc'
pg_upgrade/option.c
pgbench.c
reindexdb.c
vacuumdb.c
pg_regress.c
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On 2021-May-19, Bharath Rupireddy wrote:
> While working on [1], I found that some parts of the code is using
> strtol and atoi without checking for non-numeric junk input strings. I
> found this strange. Most of the time users provide proper numeric
> strings but there can be some scenarios where these strings are not
> user-supplied but generated by some other code which may contain
> non-numeric strings. Shouldn't the code use strtol or atoi
> appropriately and error out in such cases? One way to fix this once
> and for all is to have a common API something like int
> pg_strtol/pg_str_convert_to_int(char *opt_name, char *opt_value) which
> returns a generic message upon invalid strings ("invalid value \"%s\"
> is provided for option \"%s\", opt_name, opt_value) and returns
> integers on successful parsing.
Hi, how is this related to
https://postgr.es/m/20191028012000.GA59064@begriffs.com ?
--
Álvaro Herrera Valdivia, Chile
On Thu, May 27, 2021 at 3:05 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-May-19, Bharath Rupireddy wrote:
>
> > While working on [1], I found that some parts of the code is using
> > strtol and atoi without checking for non-numeric junk input strings. I
> > found this strange. Most of the time users provide proper numeric
> > strings but there can be some scenarios where these strings are not
> > user-supplied but generated by some other code which may contain
> > non-numeric strings. Shouldn't the code use strtol or atoi
> > appropriately and error out in such cases? One way to fix this once
> > and for all is to have a common API something like int
> > pg_strtol/pg_str_convert_to_int(char *opt_name, char *opt_value) which
> > returns a generic message upon invalid strings ("invalid value \"%s\"
> > is provided for option \"%s\", opt_name, opt_value) and returns
> > integers on successful parsing.
>
> Hi, how is this related to
> https://postgr.es/m/20191028012000.GA59064@begriffs.com ?
Thanks. The proposed approach there was to implement postgres's own
strtol i.e. string parsing, conversion to integers and use it in the
places where atoi is being used. I'm not sure how far that can go.
What I'm proposing here is to use strtol inplace of atoi to properly
detect errors in case of inputs like '1211efe', '-14adc' and so on as
atoi can't detect such errors. Thoughts?
With Regards,
Bharath Rupireddy.
On 2021-Jun-04, Bharath Rupireddy wrote: > On Thu, May 27, 2021 at 3:05 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Hi, how is this related to > > https://postgr.es/m/20191028012000.GA59064@begriffs.com ? > > Thanks. The proposed approach there was to implement postgres's own > strtol i.e. string parsing, conversion to integers and use it in the > places where atoi is being used. I'm not sure how far that can go. > What I'm proposing here is to use strtol inplace of atoi to properly > detect errors in case of inputs like '1211efe', '-14adc' and so on as > atoi can't detect such errors. Thoughts? Well, if you scroll back to Surafel's initial submission in that thread, it looks very similar in spirit to what you have here. Another thing I just noticed which I hadn't realized is that Joe Nelson's patch depends on Fabien Coelho's patch in this other thread, https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904201223040.29102@lancre which was closed as returned-with-feedback, I suppose mostly due to exhaustion/frustration at the lack of progress/interest. I would suggest that the best way forward in this area is to rebase both there patches on current master. -- Álvaro Herrera Valdivia, Chile "La virtud es el justo medio entre dos defectos" (Aristóteles)
On Fri, Jun 4, 2021 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Jun-04, Bharath Rupireddy wrote: > > > On Thu, May 27, 2021 at 3:05 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > Hi, how is this related to > > > https://postgr.es/m/20191028012000.GA59064@begriffs.com ? > > > > Thanks. The proposed approach there was to implement postgres's own > > strtol i.e. string parsing, conversion to integers and use it in the > > places where atoi is being used. I'm not sure how far that can go. > > What I'm proposing here is to use strtol inplace of atoi to properly > > detect errors in case of inputs like '1211efe', '-14adc' and so on as > > atoi can't detect such errors. Thoughts? > > Well, if you scroll back to Surafel's initial submission in that thread, > it looks very similar in spirit to what you have here. > > Another thing I just noticed which I hadn't realized is that Joe > Nelson's patch depends on Fabien Coelho's patch in this other thread, > https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904201223040.29102@lancre > which was closed as returned-with-feedback, I suppose mostly due to > exhaustion/frustration at the lack of progress/interest. > > I would suggest that the best way forward in this area is to rebase both > there patches on current master. Thanks. I will read both the threads [1], [2] and try to rebase the patches. If at all I get to rebase them, do you prefer the patches to be in this thread or in a new thread? [1] - https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904201223040.29102@lancre [2] - https://www.postgresql.org/message-id/20191028012000.GA59064@begriffs.com With Regards, Bharath Rupireddy.
On 2021-Jun-04, Bharath Rupireddy wrote: > On Fri, Jun 4, 2021 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > I would suggest that the best way forward in this area is to rebase both > > there patches on current master. > > Thanks. I will read both the threads [1], [2] and try to rebase the > patches. If at all I get to rebase them, do you prefer the patches to > be in this thread or in a new thread? Thanks, that would be helpful. This thread is a good place. -- Álvaro Herrera Valdivia, Chile
On Fri, Jun 4, 2021 at 10:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Jun-04, Bharath Rupireddy wrote: > > > On Fri, Jun 4, 2021 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > I would suggest that the best way forward in this area is to rebase both > > > there patches on current master. > > > > Thanks. I will read both the threads [1], [2] and try to rebase the > > patches. If at all I get to rebase them, do you prefer the patches to > > be in this thread or in a new thread? > > Thanks, that would be helpful. This thread is a good place. I'm unable to spend time on this work as promised. I'd be happy if someone could take it forward, although it's not critical work(IMO) that needs immediate focus. I will try to spend time maybe later this year. Regards, Bharath Rupireddy.
At Wed, 7 Jul 2021 17:40:13 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Fri, Jun 4, 2021 at 10:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2021-Jun-04, Bharath Rupireddy wrote: > > > > > On Fri, Jun 4, 2021 at 8:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > > > I would suggest that the best way forward in this area is to rebase both > > > > there patches on current master. > > > > > > Thanks. I will read both the threads [1], [2] and try to rebase the > > > patches. If at all I get to rebase them, do you prefer the patches to > > > be in this thread or in a new thread? > > > > Thanks, that would be helpful. This thread is a good place. > > I'm unable to spend time on this work as promised. I'd be happy if > someone could take it forward, although it's not critical work(IMO) > that needs immediate focus. I will try to spend time maybe later this > year. Looked through the three threads. [1] is trying to expose pg_strtoint16/32 to frontend, but I don't see much point in doing that in conjunction with [2] or this thread. Since the integral parameter values of pg-commands are in int, which the exising function strtoint() is sufficient to read. So even [2] itself doesn't need to utilize [1]. So I agree to the Bharath's point. So the attached is a standalone patch that: - doesn't contain [1], since that functions are not needed for this purpose. - basically does the same thing with [2], but uses strtoint/strtol/strtod instead of pg_strtoint16/32. - doesn't try to make error messages verbose. That results in a somewhat strange message like this but I'm not sure we should be so strict at that point. > reindexdb: error: number of parallel jobs must be at least 1: hoge - is extended to cover all usages of atoi/l/f in command line processing, which are not fully covered by [2]. (Maybe) - is extended to cover psql's meta command parameters. - is extended to cover integral environment variables. (PGPORTOLD/NEW of pg_upgrade and COLUMNS of psql). The commands emit a warning for invalid values, but I'm not sure it's worthwhile. (The second attached.) > psql: warning: ignored invalid setting of environemt variable COLUMNS: 3x - doesn't cover pgbench's meta command parameters (for speed). [1] - https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904201223040.29102@lancre [2] - https://www.postgresql.org/message-id/20191028012000.GA59064@begriffs.com regards. -- Kyotaro Horiguchi NTT Open Source Software Center From 54ed83143012473727faff9f9e64dee613cedcfe Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Thu, 8 Jul 2021 15:08:01 +0900 Subject: [PATCH 1/2] Be strict in numeric parameters on command line Some numeric command line parameters are tolerant of valid values followed by garbage like "123xyz". Be strict to reject such invalid values. Do the same for psql meta command parameters. --- src/bin/pg_amcheck/pg_amcheck.c | 6 ++- src/bin/pg_basebackup/pg_basebackup.c | 13 +++-- src/bin/pg_basebackup/pg_receivewal.c | 18 +++++-- src/bin/pg_basebackup/pg_recvlogical.c | 17 +++++-- src/bin/pg_checksums/pg_checksums.c | 7 ++- src/bin/pg_ctl/pg_ctl.c | 18 ++++++- src/bin/pg_dump/pg_dump.c | 39 ++++++++------- src/bin/pg_dump/pg_restore.c | 17 ++++--- src/bin/pg_upgrade/option.c | 21 ++++++-- src/bin/pgbench/pgbench.c | 66 ++++++++++++++++---------- src/bin/psql/command.c | 52 ++++++++++++++++++-- src/bin/scripts/reindexdb.c | 10 ++-- src/bin/scripts/vacuumdb.c | 23 +++++---- 13 files changed, 219 insertions(+), 88 deletions(-) diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index 4bde16fb4b..71a82f9b75 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -17,6 +17,7 @@ #include "catalog/pg_am_d.h" #include "catalog/pg_namespace_d.h" #include "common/logging.h" +#include "common/string.h" #include "common/username.h" #include "fe_utils/cancel.h" #include "fe_utils/option_utils.h" @@ -326,8 +327,9 @@ main(int argc, char *argv[]) append_btree_pattern(&opts.exclude, optarg, encoding); break; case 'j': - opts.jobs = atoi(optarg); - if (opts.jobs < 1) + errno = 0; + opts.jobs = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || opts.jobs < 1) { pg_log_error("number of parallel jobs must be at least 1"); exit(1); diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 8bb0acf498..29be95b96a 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2287,6 +2287,8 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP", long_options, &option_index)) != -1) { + char *endptr; + switch (c) { case 'C': @@ -2371,8 +2373,10 @@ main(int argc, char **argv) #endif break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) + errno = 0; + compresslevel = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || + compresslevel < 0 || compresslevel > 9) { pg_log_error("invalid compression level \"%s\"", optarg); exit(1); @@ -2409,8 +2413,9 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000; + if (*endptr || errno == ERANGE || standby_message_timeout < 0) { pg_log_error("invalid status interval \"%s\"", optarg); exit(1); diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index c1334fad35..7fef925b99 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -22,6 +22,7 @@ #include "access/xlog_internal.h" #include "common/file_perm.h" #include "common/logging.h" +#include "common/string.h" #include "getopt_long.h" #include "libpq-fe.h" #include "receivelog.h" @@ -520,6 +521,8 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:", long_options, &option_index)) != -1) { + char *endptr; + switch (c) { case 'D': @@ -532,7 +535,9 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) + errno = 0; + if (strtoint(optarg, &endptr, 10) <= 0 || + *endptr || errno == ERANGE) { pg_log_error("invalid port number \"%s\"", optarg); exit(1); @@ -549,8 +554,9 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000; + if (*endptr || errno == ERANGE || standby_message_timeout < 0) { pg_log_error("invalid status interval \"%s\"", optarg); exit(1); @@ -574,8 +580,10 @@ main(int argc, char **argv) verbose++; break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) + errno = 0; + compresslevel = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || + compresslevel < 0 || compresslevel > 9) { pg_log_error("invalid compression level \"%s\"", optarg); exit(1); diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 76bd153fac..7be932d025 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -23,6 +23,7 @@ #include "common/fe_memutils.h" #include "common/file_perm.h" #include "common/logging.h" +#include "common/string.h" #include "getopt_long.h" #include "libpq-fe.h" #include "libpq/pqsignal.h" @@ -732,6 +733,8 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "E:f:F:nvtd:h:p:U:wWI:o:P:s:S:", long_options, &option_index)) != -1) { + char *endptr; + switch (c) { /* general options */ @@ -739,8 +742,9 @@ main(int argc, char **argv) outfile = pg_strdup(optarg); break; case 'F': - fsync_interval = atoi(optarg) * 1000; - if (fsync_interval < 0) + errno = 0; + fsync_interval = strtoint(optarg, &endptr, 10) * 1000; + if (*endptr || errno == ERANGE || fsync_interval < 0) { pg_log_error("invalid fsync interval \"%s\"", optarg); exit(1); @@ -763,7 +767,9 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) + errno = 0; + if (strtoint(optarg, &endptr, 10) <= 0 || + *endptr || errno == ERANGE) { pg_log_error("invalid port number \"%s\"", optarg); exit(1); @@ -820,8 +826,9 @@ main(int argc, char **argv) plugin = pg_strdup(optarg); break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000; + if (*endptr || errno == ERANGE || standby_message_timeout < 0) { pg_log_error("invalid status interval \"%s\"", optarg); exit(1); diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 3c326906e2..1c4e5b9d85 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -24,6 +24,7 @@ #include "common/file_perm.h" #include "common/file_utils.h" #include "common/logging.h" +#include "common/string.h" #include "getopt_long.h" #include "pg_getopt.h" #include "storage/bufpage.h" @@ -506,6 +507,8 @@ main(int argc, char *argv[]) while ((c = getopt_long(argc, argv, "cD:deNPf:v", long_options, &option_index)) != -1) { + char *endptr; + switch (c) { case 'c': @@ -518,7 +521,9 @@ main(int argc, char *argv[]) mode = PG_MODE_ENABLE; break; case 'f': - if (atoi(optarg) == 0) + errno = 0; + if (strtoint(optarg, &endptr, 10) == 0 + || *endptr || errno == ERANGE) { pg_log_error("invalid filenode specification, must be numeric: %s", optarg); exit(1); diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 7985da0a94..78882dec48 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -2331,6 +2331,8 @@ main(int argc, char **argv) /* process command-line options */ while (optind < argc) { + char *endptr; + while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW", long_options, &option_index)) != -1) { @@ -2396,7 +2398,13 @@ main(int argc, char **argv) #endif break; case 't': - wait_seconds = atoi(optarg); + errno = 0; + wait_seconds = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || wait_seconds < 0) + { + pg_log_error("invalid timeout \"%s\"", optarg); + exit(1); + } wait_seconds_arg = true; break; case 'U': @@ -2459,7 +2467,13 @@ main(int argc, char **argv) } ctl_command = KILL_COMMAND; set_sig(argv[++optind]); - killproc = atol(argv[++optind]); + errno = 0; + killproc = strtol(argv[++optind], &endptr, 10); + if (*endptr || errno == ERANGE || killproc < 0) + { + pg_log_error("invalid process ID \"%s\"", argv[optind]); + exit(1); + } } #ifdef WIN32 else if (strcmp(argv[optind], "register") == 0) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 321152151d..793f4b3509 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -54,6 +54,7 @@ #include "catalog/pg_trigger_d.h" #include "catalog/pg_type_d.h" #include "common/connect.h" +#include "common/string.h" #include "dumputils.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" @@ -486,7 +487,19 @@ main(int argc, char **argv) break; case 'j': /* number of dump jobs */ - numWorkers = atoi(optarg); + errno = 0; + numWorkers = strtoint(optarg, &endptr, 10); + /* + * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS + * (= 64 usually) parallel jobs because that's the maximum + * limit for the WaitForMultipleObjects() call. + */ + if (*endptr || errno == ERANGE || numWorkers <= 0 +#ifdef WIN32 + || numWorkers > MAXIMUM_WAIT_OBJECTS +#endif + ) + fatal("invalid number of parallel jobs %s", optarg); break; case 'n': /* include schema(s) */ @@ -549,8 +562,10 @@ main(int argc, char **argv) break; case 'Z': /* Compression Level */ - compressLevel = atoi(optarg); - if (compressLevel < 0 || compressLevel > 9) + errno = 0; + compressLevel = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || + compressLevel < 0 || compressLevel > 9) { pg_log_error("compression level must be in range 0..9"); exit_nicely(1); @@ -587,8 +602,10 @@ main(int argc, char **argv) case 8: have_extra_float_digits = true; - extra_float_digits = atoi(optarg); - if (extra_float_digits < -15 || extra_float_digits > 3) + errno = 0; + extra_float_digits = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || + extra_float_digits < -15 || extra_float_digits > 3) { pg_log_error("extra_float_digits must be in range -15..3"); exit_nicely(1); @@ -719,18 +736,6 @@ main(int argc, char **argv) if (!plainText) dopt.outputCreateDB = 1; - /* - * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually) - * parallel jobs because that's the maximum limit for the - * WaitForMultipleObjects() call. - */ - if (numWorkers <= 0 -#ifdef WIN32 - || numWorkers > MAXIMUM_WAIT_OBJECTS -#endif - ) - fatal("invalid number of parallel jobs"); - /* Parallel backup only in the directory archive format so far */ if (archiveFormat != archDirectory && numWorkers > 1) fatal("parallel backup only supported by the directory format"); diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index 589b4aed53..285a09aaac 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -39,6 +39,7 @@ *------------------------------------------------------------------------- */ #include "postgres_fe.h" +#include "common/string.h" #include <ctype.h> #ifdef HAVE_TERMIOS_H @@ -151,6 +152,8 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1", cmdopts, NULL)) != -1) { + char *endptr; + switch (c) { case 'a': /* Dump data only */ @@ -181,7 +184,13 @@ main(int argc, char **argv) break; case 'j': /* number of restore jobs */ - numWorkers = atoi(optarg); + errno = 0; + numWorkers = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || numWorkers <= 0) + { + pg_log_error("invalid number of parallel jobs"); + exit(1); + } break; case 'l': /* Dump the TOC summary */ @@ -344,12 +353,6 @@ main(int argc, char **argv) exit_nicely(1); } - if (numWorkers <= 0) - { - pg_log_error("invalid number of parallel jobs"); - exit(1); - } - /* See comments in pg_dump.c */ #ifdef WIN32 if (numWorkers > MAXIMUM_WAIT_OBJECTS) diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 64bbda5650..357ba04f4f 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -104,6 +104,8 @@ parseCommandLine(int argc, char *argv[]) while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v", long_options, &optindex)) != -1) { + char *endptr; + switch (option) { case 'b': @@ -127,7 +129,12 @@ parseCommandLine(int argc, char *argv[]) break; case 'j': - user_opts.jobs = atoi(optarg); + errno = 0; + user_opts.jobs = strtoint(optarg, &endptr, 10); + /**/ + if (*endptr || errno == ERANGE) + pg_fatal("invalid number of jobs %s\n", optarg); + break; case 'k': @@ -166,13 +173,17 @@ parseCommandLine(int argc, char *argv[]) * supported on all old/new versions (added in PG 9.2). */ case 'p': - if ((old_cluster.port = atoi(optarg)) <= 0) - pg_fatal("invalid old port number\n"); + errno = 0; + if ((old_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 || + *endptr || errno == ERANGE) + pg_fatal("invalid old port number %s\n", optarg); break; case 'P': - if ((new_cluster.port = atoi(optarg)) <= 0) - pg_fatal("invalid new port number\n"); + errno = 0; + if ((new_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 || + *endptr || errno == ERANGE) + pg_fatal("invalid new port number %s\n", optarg); break; case 'r': diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 4aeccd93af..4020347585 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -5838,6 +5838,7 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1) { char *script; + char *endptr; switch (c) { @@ -5869,8 +5870,9 @@ main(int argc, char **argv) break; case 'c': benchmarking_option_set = true; - nclients = atoi(optarg); - if (nclients <= 0) + errno = 0; + nclients = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || nclients <= 0) { pg_log_fatal("invalid number of clients: \"%s\"", optarg); exit(1); @@ -5896,8 +5898,9 @@ main(int argc, char **argv) break; case 'j': /* jobs */ benchmarking_option_set = true; - nthreads = atoi(optarg); - if (nthreads <= 0) + errno = 0; + nthreads = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || nthreads <= 0) { pg_log_fatal("invalid number of threads: \"%s\"", optarg); exit(1); @@ -5920,8 +5923,9 @@ main(int argc, char **argv) break; case 's': scale_given = true; - scale = atoi(optarg); - if (scale <= 0) + errno = 0; + scale = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || scale <= 0) { pg_log_fatal("invalid scaling factor: \"%s\"", optarg); exit(1); @@ -5929,8 +5933,9 @@ main(int argc, char **argv) break; case 't': benchmarking_option_set = true; - nxacts = atoi(optarg); - if (nxacts <= 0) + errno = 0; + nxacts = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || nxacts <= 0) { pg_log_fatal("invalid number of transactions: \"%s\"", optarg); exit(1); @@ -5938,8 +5943,9 @@ main(int argc, char **argv) break; case 'T': benchmarking_option_set = true; - duration = atoi(optarg); - if (duration <= 0) + errno = 0; + duration = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || duration <= 0) { pg_log_fatal("invalid duration: \"%s\"", optarg); exit(1); @@ -6001,8 +6007,10 @@ main(int argc, char **argv) break; case 'F': initialization_option_set = true; - fillfactor = atoi(optarg); - if (fillfactor < 10 || fillfactor > 100) + errno = 0; + fillfactor = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || + fillfactor < 10 || fillfactor > 100) { pg_log_fatal("invalid fillfactor: \"%s\"", optarg); exit(1); @@ -6021,8 +6029,9 @@ main(int argc, char **argv) break; case 'P': benchmarking_option_set = true; - progress = atoi(optarg); - if (progress <= 0) + errno = 0; + progress = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || progress <= 0) { pg_log_fatal("invalid thread progress delay: \"%s\"", optarg); exit(1); @@ -6031,11 +6040,13 @@ main(int argc, char **argv) case 'R': { /* get a double from the beginning of option value */ - double throttle_value = atof(optarg); + double throttle_value; + errno = 0; + throttle_value = strtod(optarg, &endptr); benchmarking_option_set = true; - if (throttle_value <= 0.0) + if (*endptr || errno == ERANGE || throttle_value <= 0.0) { pg_log_fatal("invalid rate limit: \"%s\"", optarg); exit(1); @@ -6046,9 +6057,12 @@ main(int argc, char **argv) break; case 'L': { - double limit_ms = atof(optarg); + double limit_ms; - if (limit_ms <= 0.0) + errno = 0; + limit_ms = strtod(optarg, &endptr); + + if (*endptr || errno == ERANGE || limit_ms <= 0.0) { pg_log_fatal("invalid latency limit: \"%s\"", optarg); exit(1); @@ -6071,8 +6085,10 @@ main(int argc, char **argv) break; case 4: /* sampling-rate */ benchmarking_option_set = true; - sample_rate = atof(optarg); - if (sample_rate <= 0.0 || sample_rate > 1.0) + errno = 0; + sample_rate = strtod(optarg, &endptr); + if (*endptr || errno == ERANGE || + sample_rate <= 0.0 || sample_rate > 1.0) { pg_log_fatal("invalid sampling rate: \"%s\"", optarg); exit(1); @@ -6080,8 +6096,9 @@ main(int argc, char **argv) break; case 5: /* aggregate-interval */ benchmarking_option_set = true; - agg_interval = atoi(optarg); - if (agg_interval <= 0) + errno = 0; + agg_interval = strtod(optarg, &endptr); + if (*endptr || errno == ERANGE || agg_interval <= 0) { pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg); exit(1); @@ -6117,8 +6134,9 @@ main(int argc, char **argv) break; case 11: /* partitions */ initialization_option_set = true; - partitions = atoi(optarg); - if (partitions < 0) + errno = 0; + partitions = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || partitions < 0) { pg_log_fatal("invalid number of partitions: \"%s\"", optarg); exit(1); diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 543401c6d6..aaed986ae1 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1039,8 +1039,11 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch, } if (ln) { - lineno = atoi(ln); - if (lineno < 1) + char *endptr; + + errno = 0; + lineno = strtoint(ln, &endptr, 10); + if (*endptr || errno == ERANGE || lineno < 1) { pg_log_error("invalid line number: %s", ln); status = PSQL_CMD_ERROR; @@ -4283,7 +4286,21 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet) else if (strcmp(param, "border") == 0) { if (value) - popt->topt.border = atoi(value); + { + char *endptr; + int new_value; + + errno = 0; + new_value = strtoint(value, &endptr, 10); + if (*endptr || errno == ERANGE || + new_value < 0 || new_value > 65535) + { + pg_log_error("\\pset: border is invalid or out of range"); + return false; + } + + popt->topt.border = new_value; + } } /* set expanded/vertical mode */ @@ -4439,7 +4456,20 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet) else if (strcmp(param, "pager_min_lines") == 0) { if (value) - popt->topt.pager_min_lines = atoi(value); + { + char *endptr; + int new_value; + + errno = 0; + new_value = strtoint(value, &endptr, 10); + if (*endptr || errno == ERANGE || new_value < 0) + { + pg_log_error("\\pset: pager_min_lines is invalid or out of range"); + return false; + } + + popt->topt.pager_min_lines = new_value; + } } /* disable "(x rows)" footer */ @@ -4455,7 +4485,19 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet) else if (strcmp(param, "columns") == 0) { if (value) - popt->topt.columns = atoi(value); + { + char *endptr; + int new_value; + + errno = 0; + new_value = strtoint(value, &endptr, 10); + if (*endptr || errno == ERANGE || new_value < 0) + { + pg_log_error("\\pset: column is invalid or out of range"); + return false; + } + popt->topt.columns = new_value; + } } else { diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index fc0681538a..baa68d58d8 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -15,6 +15,7 @@ #include "common.h" #include "common/connect.h" #include "common/logging.h" +#include "common/string.h" #include "fe_utils/cancel.h" #include "fe_utils/option_utils.h" #include "fe_utils/parallel_slot.h" @@ -109,6 +110,8 @@ main(int argc, char *argv[]) /* process command-line options */ while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1) { + char *endptr; + switch (c) { case 'h': @@ -151,10 +154,11 @@ main(int argc, char *argv[]) simple_string_list_append(&indexes, optarg); break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + errno = 0; + concurrentCons = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || concurrentCons <= 0) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("number of parallel jobs must be at least 1: %s", optarg); exit(1); } break; diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 61974baa78..93b563998a 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -17,6 +17,7 @@ #include "common.h" #include "common/connect.h" #include "common/logging.h" +#include "common/string.h" #include "fe_utils/cancel.h" #include "fe_utils/option_utils.h" #include "fe_utils/parallel_slot.h" @@ -141,6 +142,8 @@ main(int argc, char *argv[]) while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:P:", long_options, &optindex)) != -1) { + char *endptr; + switch (c) { case 'h': @@ -192,16 +195,18 @@ main(int argc, char *argv[]) vacopts.verbose = true; break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + errno = 0; + concurrentCons = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || concurrentCons <= 0) { pg_log_error("number of parallel jobs must be at least 1"); exit(1); } break; case 'P': - vacopts.parallel_workers = atoi(optarg); - if (vacopts.parallel_workers < 0) + errno = 0; + vacopts.parallel_workers = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || vacopts.parallel_workers < 0) { pg_log_error("parallel workers for vacuum must be greater than or equal to zero"); exit(1); @@ -220,16 +225,18 @@ main(int argc, char *argv[]) vacopts.skip_locked = true; break; case 6: - vacopts.min_xid_age = atoi(optarg); - if (vacopts.min_xid_age <= 0) + errno = 0; + vacopts.min_xid_age = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || vacopts.min_xid_age <= 0) { pg_log_error("minimum transaction ID age must be at least 1"); exit(1); } break; case 7: - vacopts.min_mxid_age = atoi(optarg); - if (vacopts.min_mxid_age <= 0) + errno = 0; + vacopts.min_mxid_age = strtoint(optarg, &endptr, 10); + if (*endptr || errno == ERANGE || vacopts.min_mxid_age <= 0) { pg_log_error("minimum multixact ID age must be at least 1"); exit(1); -- 2.27.0 From 17d541ccc9cd55ff7b547d02b72099e9a26268dd Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Thu, 8 Jul 2021 17:10:19 +0900 Subject: [PATCH 2/2] Make complain for invalid numeirc values in environemnt variables Postgresql commands that read environment variables for integer values are tolerant of trailing garbages like '5432xyz'. Ignore such values and make warning on it. --- src/bin/pg_upgrade/option.c | 32 ++++++++++++++++++++++++++++++-- src/bin/psql/startup.c | 17 ++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 357ba04f4f..95a979004d 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -71,8 +71,36 @@ parseCommandLine(int argc, char *argv[]) os_info.progname = get_progname(argv[0]); /* Process libpq env. variables; load values here for usage() output */ - old_cluster.port = getenv("PGPORTOLD") ? atoi(getenv("PGPORTOLD")) : DEF_PGUPORT; - new_cluster.port = getenv("PGPORTNEW") ? atoi(getenv("PGPORTNEW")) : DEF_PGUPORT; + old_cluster.port = new_cluster.port = DEF_PGUPORT; + if (getenv("PGPORTOLD")) + { + char *endptr; + int port; + + errno = 0; + port = strtoint(getenv("PGPORTOLD"), &endptr, 10); + if (*endptr || errno == ERANGE || port < 0) + pg_log(PG_WARNING, + "%s: ignored invalid setting of environment variable PGPORTOLD: %s\n", + os_info.progname, getenv("PGPORTOLD")); + else + old_cluster.port = port; + } + + if (getenv("PGPORTNEW")) + { + char *endptr; + int port; + + errno = 0; + port = strtoint(getenv("PGPORTNEW"), &endptr, 10); + if (*endptr || errno == ERANGE || port < 0) + pg_log(PG_WARNING, + "%s: ignored invalid setting of environment variable PGPORTNEW: %s\n", + os_info.progname, getenv("PGPORTNEW")); + else + new_cluster.port = port; + } os_user_effective_id = get_user_info(&os_info.user); /* we override just the database user name; we got the OS id above */ diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index 110906a4e9..4378c5daa3 100644 --- a/src/bin/psql/startup.c +++ b/src/bin/psql/startup.c @@ -174,7 +174,22 @@ main(int argc, char *argv[]) refresh_utf8format(&(pset.popt.topt)); /* We must get COLUMNS here before readline() sets it */ - pset.popt.topt.env_columns = getenv("COLUMNS") ? atoi(getenv("COLUMNS")) : 0; + pset.popt.topt.env_columns = 0; + if (getenv("COLUMNS")) + { + char *endptr; + int cols; + + errno = 0; + cols = strtoint(getenv("COLUMNS"), &endptr, 10); + if (*endptr || errno == ERANGE || cols < 0) + { + pg_log_warning("ignored invalid setting of environemt variable COLUMNS: %s", + getenv("COLUMNS")); + } + + pset.popt.topt.env_columns = cols; + } pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout))); -- 2.27.0
Thank you for the comments.
At Fri, 9 Jul 2021 10:29:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Thu, Jul 08, 2021 at 05:30:23PM +0900, Kyotaro Horiguchi wrote:
> > [1] is trying to expose pg_strtoint16/32 to frontend, but I don't see
> > much point in doing that in conjunction with [2] or this thread. Since
> > the integral parameter values of pg-commands are in int, which the
> > exising function strtoint() is sufficient to read. So even [2] itself
> > doesn't need to utilize [1].
>
> It sounds sensible from here to just use strtoint(), some strtol(),
> son strtod() and call it a day as these are already available.
Thanks.
> > - wait_seconds = atoi(optarg);
> > + errno = 0;
> > + wait_seconds = strtoint(optarg, &endptr, 10);
> > + if (*endptr || errno == ERANGE || wait_seconds < 0)
> > + {
> > + pg_log_error("invalid timeout \"%s\"", optarg);
> > + exit(1);
> > + }
> > [ ... ]
> > - killproc = atol(argv[++optind]);
> > + errno = 0;
> > + killproc = strtol(argv[++optind], &endptr, 10);
> > + if (*endptr || errno == ERANGE || killproc < 0)
> > + {
> > + pg_log_error("invalid process ID \"%s\"", argv[optind]);
> > + exit(1);
> > + }
>
> Er, wait. We've actually allowed negative values for pg_ctl
> --timeout or the subcommand kill!?
For killproc, leading minus sign suggests that it is an command line
option. Since pg_ctl doesn't have such an option, that values is
recognized as invalid *options*, even with the additional check. The
additional check is useless in that sense. My intension is just to
make the restriction explicit so I won't feel sad even if it should be
removed.
$ pg_ctl kill HUP -12345
cg_ctl: infalid option -- '1'
--timeout accepts values less than 1, which values cause the command
ends with the timeout error before checking for the ending state. Not
destructive but useless as a behavior. We have two choices here. One
is simply inhibiting zero or negative timeouts, and another is
allowing zero as timeout and giving it the same meaning to
--no-wait. The former is strictly right behavior but the latter is
casual and convenient. I took the former way in this version.
$ pg_ctl -w -t 0 start
pg_ctl: error: invalid timeout value "0", use --no-wait to finish without waiting
The same message is shown for non-decimal values but that would not matters.
> > case 'j':
> > - user_opts.jobs = atoi(optarg);
> > + errno = 0;
> > + user_opts.jobs = strtoint(optarg, &endptr, 10);
> > + /**/
> > + if (*endptr || errno == ERANGE)
> > + pg_fatal("invalid number of jobs %s\n", optarg);
> > +
> > break;
>
> This one in pg_upgrade is incomplete. Perhaps the missing comment
> should tell that negative job values are checked later on?
Zero or negative job numbers mean non-parallel mode and don't lead to
an error. If we don't need to preserve that behavior (I personally
don't think it is ether useful and/or breaks someone's existing
usage.), it is sensible to do range-check here.
I noticed that I overlooked PGCTLTIMEOUT.
The attached is:
- disallowed less-than-one numbers as jobs in pg_upgrade.
- disallowed less-than-one timeout in pg_ctl
- Use strtoint for PGCTLTIMEOUT in pg_ctl is
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From 0818de16f9febea3d90ae0404b4f5b3643f6cbe0 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 15:08:01 +0900
Subject: [PATCH v2 1/2] Be strict in numeric parameters on command line
Some numeric command line parameters are tolerant of valid values
followed by garbage like "123xyz". Be strict to reject such invalid
values. Do the same for psql meta command parameters.
---
src/bin/pg_amcheck/pg_amcheck.c | 6 ++-
src/bin/pg_basebackup/pg_basebackup.c | 13 +++--
src/bin/pg_basebackup/pg_receivewal.c | 18 +++++--
src/bin/pg_basebackup/pg_recvlogical.c | 17 +++++--
src/bin/pg_checksums/pg_checksums.c | 7 ++-
src/bin/pg_ctl/pg_ctl.c | 18 ++++++-
src/bin/pg_dump/pg_dump.c | 39 ++++++++-------
src/bin/pg_dump/pg_restore.c | 17 ++++---
src/bin/pg_upgrade/option.c | 21 ++++++--
src/bin/pgbench/pgbench.c | 66 ++++++++++++++++----------
src/bin/psql/command.c | 52 ++++++++++++++++++--
src/bin/scripts/reindexdb.c | 10 ++--
src/bin/scripts/vacuumdb.c | 23 +++++----
13 files changed, 219 insertions(+), 88 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..71a82f9b75 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -17,6 +17,7 @@
#include "catalog/pg_am_d.h"
#include "catalog/pg_namespace_d.h"
#include "common/logging.h"
+#include "common/string.h"
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
@@ -326,8 +327,9 @@ main(int argc, char *argv[])
append_btree_pattern(&opts.exclude, optarg, encoding);
break;
case 'j':
- opts.jobs = atoi(optarg);
- if (opts.jobs < 1)
+ errno = 0;
+ opts.jobs = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || opts.jobs < 1)
{
pg_log_error("number of parallel jobs must be at least 1");
exit(1);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf498..29be95b96a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2287,6 +2287,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'C':
@@ -2371,8 +2373,10 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ errno = 0;
+ compresslevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compresslevel < 0 || compresslevel > 9)
{
pg_log_error("invalid compression level \"%s\"", optarg);
exit(1);
@@ -2409,8 +2413,9 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || standby_message_timeout < 0)
{
pg_log_error("invalid status interval \"%s\"", optarg);
exit(1);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index c1334fad35..7fef925b99 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -22,6 +22,7 @@
#include "access/xlog_internal.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "receivelog.h"
@@ -520,6 +521,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'D':
@@ -532,7 +535,9 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ errno = 0;
+ if (strtoint(optarg, &endptr, 10) <= 0 ||
+ *endptr || errno == ERANGE)
{
pg_log_error("invalid port number \"%s\"", optarg);
exit(1);
@@ -549,8 +554,9 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || standby_message_timeout < 0)
{
pg_log_error("invalid status interval \"%s\"", optarg);
exit(1);
@@ -574,8 +580,10 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ errno = 0;
+ compresslevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compresslevel < 0 || compresslevel > 9)
{
pg_log_error("invalid compression level \"%s\"", optarg);
exit(1);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 76bd153fac..7be932d025 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -23,6 +23,7 @@
#include "common/fe_memutils.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -732,6 +733,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "E:f:F:nvtd:h:p:U:wWI:o:P:s:S:",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
/* general options */
@@ -739,8 +742,9 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ errno = 0;
+ fsync_interval = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || fsync_interval < 0)
{
pg_log_error("invalid fsync interval \"%s\"", optarg);
exit(1);
@@ -763,7 +767,9 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ errno = 0;
+ if (strtoint(optarg, &endptr, 10) <= 0 ||
+ *endptr || errno == ERANGE)
{
pg_log_error("invalid port number \"%s\"", optarg);
exit(1);
@@ -820,8 +826,9 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr || errno == ERANGE || standby_message_timeout < 0)
{
pg_log_error("invalid status interval \"%s\"", optarg);
exit(1);
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3c326906e2..1c4e5b9d85 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -24,6 +24,7 @@
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -506,6 +507,8 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "cD:deNPf:v", long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'c':
@@ -518,7 +521,9 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
case 'f':
- if (atoi(optarg) == 0)
+ errno = 0;
+ if (strtoint(optarg, &endptr, 10) == 0
+ || *endptr || errno == ERANGE)
{
pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
exit(1);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..d6a39182cf 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2331,6 +2331,8 @@ main(int argc, char **argv)
/* process command-line options */
while (optind < argc)
{
+ char *endptr;
+
while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
long_options, &option_index)) != -1)
{
@@ -2396,7 +2398,13 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ errno = 0;
+ wait_seconds = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || wait_seconds < 1)
+ {
+ pg_log_error("invalid timeout value \"%s\", use --no-wait to finish without waiting",
optarg);
+ exit(1);
+ }
wait_seconds_arg = true;
break;
case 'U':
@@ -2459,7 +2467,13 @@ main(int argc, char **argv)
}
ctl_command = KILL_COMMAND;
set_sig(argv[++optind]);
- killproc = atol(argv[++optind]);
+ errno = 0;
+ killproc = strtol(argv[++optind], &endptr, 10);
+ if (*endptr || errno == ERANGE || killproc < 0)
+ {
+ pg_log_error("invalid process ID \"%s\"", argv[optind]);
+ exit(1);
+ }
}
#ifdef WIN32
else if (strcmp(argv[optind], "register") == 0)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 321152151d..793f4b3509 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -54,6 +54,7 @@
#include "catalog/pg_trigger_d.h"
#include "catalog/pg_type_d.h"
#include "common/connect.h"
+#include "common/string.h"
#include "dumputils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
@@ -486,7 +487,19 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ errno = 0;
+ numWorkers = strtoint(optarg, &endptr, 10);
+ /*
+ * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS
+ * (= 64 usually) parallel jobs because that's the maximum
+ * limit for the WaitForMultipleObjects() call.
+ */
+ if (*endptr || errno == ERANGE || numWorkers <= 0
+#ifdef WIN32
+ || numWorkers > MAXIMUM_WAIT_OBJECTS
+#endif
+ )
+ fatal("invalid number of parallel jobs %s", optarg);
break;
case 'n': /* include schema(s) */
@@ -549,8 +562,10 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ errno = 0;
+ compressLevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compressLevel < 0 || compressLevel > 9)
{
pg_log_error("compression level must be in range 0..9");
exit_nicely(1);
@@ -587,8 +602,10 @@ main(int argc, char **argv)
case 8:
have_extra_float_digits = true;
- extra_float_digits = atoi(optarg);
- if (extra_float_digits < -15 || extra_float_digits > 3)
+ errno = 0;
+ extra_float_digits = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ extra_float_digits < -15 || extra_float_digits > 3)
{
pg_log_error("extra_float_digits must be in range -15..3");
exit_nicely(1);
@@ -719,18 +736,6 @@ main(int argc, char **argv)
if (!plainText)
dopt.outputCreateDB = 1;
- /*
- * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
- * parallel jobs because that's the maximum limit for the
- * WaitForMultipleObjects() call.
- */
- if (numWorkers <= 0
-#ifdef WIN32
- || numWorkers > MAXIMUM_WAIT_OBJECTS
-#endif
- )
- fatal("invalid number of parallel jobs");
-
/* Parallel backup only in the directory archive format so far */
if (archiveFormat != archDirectory && numWorkers > 1)
fatal("parallel backup only supported by the directory format");
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 589b4aed53..285a09aaac 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -39,6 +39,7 @@
*-------------------------------------------------------------------------
*/
#include "postgres_fe.h"
+#include "common/string.h"
#include <ctype.h>
#ifdef HAVE_TERMIOS_H
@@ -151,6 +152,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
cmdopts, NULL)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'a': /* Dump data only */
@@ -181,7 +184,13 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ errno = 0;
+ numWorkers = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || numWorkers <= 0)
+ {
+ pg_log_error("invalid number of parallel jobs");
+ exit(1);
+ }
break;
case 'l': /* Dump the TOC summary */
@@ -344,12 +353,6 @@ main(int argc, char **argv)
exit_nicely(1);
}
- if (numWorkers <= 0)
- {
- pg_log_error("invalid number of parallel jobs");
- exit(1);
- }
-
/* See comments in pg_dump.c */
#ifdef WIN32
if (numWorkers > MAXIMUM_WAIT_OBJECTS)
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 64bbda5650..f96f0d1e2a 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -104,6 +104,8 @@ parseCommandLine(int argc, char *argv[])
while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (option)
{
case 'b':
@@ -127,7 +129,12 @@ parseCommandLine(int argc, char *argv[])
break;
case 'j':
- user_opts.jobs = atoi(optarg);
+ errno = 0;
+ user_opts.jobs = strtoint(optarg, &endptr, 10);
+ /**/
+ if (*endptr || errno == ERANGE || user_opts.jobs < 1)
+ pg_fatal("invalid number of jobs %s\n", optarg);
+
break;
case 'k':
@@ -166,13 +173,17 @@ parseCommandLine(int argc, char *argv[])
* supported on all old/new versions (added in PG 9.2).
*/
case 'p':
- if ((old_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid old port number\n");
+ errno = 0;
+ if ((old_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 ||
+ *endptr || errno == ERANGE)
+ pg_fatal("invalid old port number %s\n", optarg);
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid new port number\n");
+ errno = 0;
+ if ((new_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 ||
+ *endptr || errno == ERANGE)
+ pg_fatal("invalid new port number %s\n", optarg);
break;
case 'r':
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4aeccd93af..4020347585 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5838,6 +5838,7 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) !=
-1)
{
char *script;
+ char *endptr;
switch (c)
{
@@ -5869,8 +5870,9 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
+ errno = 0;
+ nclients = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || nclients <= 0)
{
pg_log_fatal("invalid number of clients: \"%s\"", optarg);
exit(1);
@@ -5896,8 +5898,9 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ errno = 0;
+ nthreads = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || nthreads <= 0)
{
pg_log_fatal("invalid number of threads: \"%s\"", optarg);
exit(1);
@@ -5920,8 +5923,9 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ errno = 0;
+ scale = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || scale <= 0)
{
pg_log_fatal("invalid scaling factor: \"%s\"", optarg);
exit(1);
@@ -5929,8 +5933,9 @@ main(int argc, char **argv)
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ errno = 0;
+ nxacts = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || nxacts <= 0)
{
pg_log_fatal("invalid number of transactions: \"%s\"", optarg);
exit(1);
@@ -5938,8 +5943,9 @@ main(int argc, char **argv)
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
+ errno = 0;
+ duration = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || duration <= 0)
{
pg_log_fatal("invalid duration: \"%s\"", optarg);
exit(1);
@@ -6001,8 +6007,10 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ errno = 0;
+ fillfactor = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ fillfactor < 10 || fillfactor > 100)
{
pg_log_fatal("invalid fillfactor: \"%s\"", optarg);
exit(1);
@@ -6021,8 +6029,9 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ errno = 0;
+ progress = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || progress <= 0)
{
pg_log_fatal("invalid thread progress delay: \"%s\"", optarg);
exit(1);
@@ -6031,11 +6040,13 @@ main(int argc, char **argv)
case 'R':
{
/* get a double from the beginning of option value */
- double throttle_value = atof(optarg);
+ double throttle_value;
+ errno = 0;
+ throttle_value = strtod(optarg, &endptr);
benchmarking_option_set = true;
- if (throttle_value <= 0.0)
+ if (*endptr || errno == ERANGE || throttle_value <= 0.0)
{
pg_log_fatal("invalid rate limit: \"%s\"", optarg);
exit(1);
@@ -6046,9 +6057,12 @@ main(int argc, char **argv)
break;
case 'L':
{
- double limit_ms = atof(optarg);
+ double limit_ms;
- if (limit_ms <= 0.0)
+ errno = 0;
+ limit_ms = strtod(optarg, &endptr);
+
+ if (*endptr || errno == ERANGE || limit_ms <= 0.0)
{
pg_log_fatal("invalid latency limit: \"%s\"", optarg);
exit(1);
@@ -6071,8 +6085,10 @@ main(int argc, char **argv)
break;
case 4: /* sampling-rate */
benchmarking_option_set = true;
- sample_rate = atof(optarg);
- if (sample_rate <= 0.0 || sample_rate > 1.0)
+ errno = 0;
+ sample_rate = strtod(optarg, &endptr);
+ if (*endptr || errno == ERANGE ||
+ sample_rate <= 0.0 || sample_rate > 1.0)
{
pg_log_fatal("invalid sampling rate: \"%s\"", optarg);
exit(1);
@@ -6080,8 +6096,9 @@ main(int argc, char **argv)
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ errno = 0;
+ agg_interval = strtod(optarg, &endptr);
+ if (*endptr || errno == ERANGE || agg_interval <= 0)
{
pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg);
exit(1);
@@ -6117,8 +6134,9 @@ main(int argc, char **argv)
break;
case 11: /* partitions */
initialization_option_set = true;
- partitions = atoi(optarg);
- if (partitions < 0)
+ errno = 0;
+ partitions = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || partitions < 0)
{
pg_log_fatal("invalid number of partitions: \"%s\"", optarg);
exit(1);
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 543401c6d6..aaed986ae1 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1039,8 +1039,11 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (ln)
{
- lineno = atoi(ln);
- if (lineno < 1)
+ char *endptr;
+
+ errno = 0;
+ lineno = strtoint(ln, &endptr, 10);
+ if (*endptr || errno == ERANGE || lineno < 1)
{
pg_log_error("invalid line number: %s", ln);
status = PSQL_CMD_ERROR;
@@ -4283,7 +4286,21 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "border") == 0)
{
if (value)
- popt->topt.border = atoi(value);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ new_value < 0 || new_value > 65535)
+ {
+ pg_log_error("\\pset: border is invalid or out of range");
+ return false;
+ }
+
+ popt->topt.border = new_value;
+ }
}
/* set expanded/vertical mode */
@@ -4439,7 +4456,20 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "pager_min_lines") == 0)
{
if (value)
- popt->topt.pager_min_lines = atoi(value);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr || errno == ERANGE || new_value < 0)
+ {
+ pg_log_error("\\pset: pager_min_lines is invalid or out of range");
+ return false;
+ }
+
+ popt->topt.pager_min_lines = new_value;
+ }
}
/* disable "(x rows)" footer */
@@ -4455,7 +4485,19 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "columns") == 0)
{
if (value)
- popt->topt.columns = atoi(value);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr || errno == ERANGE || new_value < 0)
+ {
+ pg_log_error("\\pset: column is invalid or out of range");
+ return false;
+ }
+ popt->topt.columns = new_value;
+ }
}
else
{
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc0681538a..baa68d58d8 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -15,6 +15,7 @@
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
#include "fe_utils/parallel_slot.h"
@@ -109,6 +110,8 @@ main(int argc, char *argv[])
/* process command-line options */
while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'h':
@@ -151,10 +154,11 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ errno = 0;
+ concurrentCons = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || concurrentCons <= 0)
{
- pg_log_error("number of parallel jobs must be at least 1");
+ pg_log_error("number of parallel jobs must be at least 1: %s", optarg);
exit(1);
}
break;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 61974baa78..93b563998a 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -17,6 +17,7 @@
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
#include "fe_utils/parallel_slot.h"
@@ -141,6 +142,8 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:P:", long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'h':
@@ -192,16 +195,18 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ errno = 0;
+ concurrentCons = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || concurrentCons <= 0)
{
pg_log_error("number of parallel jobs must be at least 1");
exit(1);
}
break;
case 'P':
- vacopts.parallel_workers = atoi(optarg);
- if (vacopts.parallel_workers < 0)
+ errno = 0;
+ vacopts.parallel_workers = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || vacopts.parallel_workers < 0)
{
pg_log_error("parallel workers for vacuum must be greater than or equal to zero");
exit(1);
@@ -220,16 +225,18 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
+ errno = 0;
+ vacopts.min_xid_age = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || vacopts.min_xid_age <= 0)
{
pg_log_error("minimum transaction ID age must be at least 1");
exit(1);
}
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
+ errno = 0;
+ vacopts.min_mxid_age = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE || vacopts.min_mxid_age <= 0)
{
pg_log_error("minimum multixact ID age must be at least 1");
exit(1);
--
2.27.0
From 039490f1bb8a866be62276fdfb50c322d7686351 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 17:10:19 +0900
Subject: [PATCH v2 2/2] Make complain for invalid numeirc values in
environemnt variables
Postgresql commands that read environment variables for integer values
are tolerant of trailing garbages like '5432xyz'. Ignore such values
and make warning on it.
---
src/bin/pg_ctl/pg_ctl.c | 12 +++++++++++-
src/bin/pg_upgrade/option.c | 32 ++++++++++++++++++++++++++++++--
src/bin/psql/startup.c | 17 ++++++++++++++++-
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index d6a39182cf..3025eb31bd 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2318,7 +2318,17 @@ main(int argc, char **argv)
env_wait = getenv("PGCTLTIMEOUT");
if (env_wait != NULL)
- wait_seconds = atoi(env_wait);
+ {
+ char *endptr;
+ int tmp_wait;
+
+ errno = 0;
+ tmp_wait = strtoint(env_wait, &endptr, 10);
+ if (*endptr || errno == ERANGE || tmp_wait < 1)
+ write_stderr(_("%s: ignored invalid setting of environment variable PGCTLTIMEOUT: %s\n"), progname,
env_wait);
+ else
+ wait_seconds = tmp_wait;
+ }
/*
* 'Action' can be before or after args so loop over both. Some
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index f96f0d1e2a..869f119456 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -71,8 +71,36 @@ parseCommandLine(int argc, char *argv[])
os_info.progname = get_progname(argv[0]);
/* Process libpq env. variables; load values here for usage() output */
- old_cluster.port = getenv("PGPORTOLD") ? atoi(getenv("PGPORTOLD")) : DEF_PGUPORT;
- new_cluster.port = getenv("PGPORTNEW") ? atoi(getenv("PGPORTNEW")) : DEF_PGUPORT;
+ old_cluster.port = new_cluster.port = DEF_PGUPORT;
+ if (getenv("PGPORTOLD"))
+ {
+ char *endptr;
+ int port;
+
+ errno = 0;
+ port = strtoint(getenv("PGPORTOLD"), &endptr, 10);
+ if (*endptr || errno == ERANGE || port < 0)
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTOLD: %s\n",
+ os_info.progname, getenv("PGPORTOLD"));
+ else
+ old_cluster.port = port;
+ }
+
+ if (getenv("PGPORTNEW"))
+ {
+ char *endptr;
+ int port;
+
+ errno = 0;
+ port = strtoint(getenv("PGPORTNEW"), &endptr, 10);
+ if (*endptr || errno == ERANGE || port < 0)
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTNEW: %s\n",
+ os_info.progname, getenv("PGPORTNEW"));
+ else
+ new_cluster.port = port;
+ }
os_user_effective_id = get_user_info(&os_info.user);
/* we override just the database user name; we got the OS id above */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 110906a4e9..4378c5daa3 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -174,7 +174,22 @@ main(int argc, char *argv[])
refresh_utf8format(&(pset.popt.topt));
/* We must get COLUMNS here before readline() sets it */
- pset.popt.topt.env_columns = getenv("COLUMNS") ? atoi(getenv("COLUMNS")) : 0;
+ pset.popt.topt.env_columns = 0;
+ if (getenv("COLUMNS"))
+ {
+ char *endptr;
+ int cols;
+
+ errno = 0;
+ cols = strtoint(getenv("COLUMNS"), &endptr, 10);
+ if (*endptr || errno == ERANGE || cols < 0)
+ {
+ pg_log_warning("ignored invalid setting of environemt variable COLUMNS: %s",
+ getenv("COLUMNS"));
+ }
+
+ pset.popt.topt.env_columns = cols;
+ }
pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout)));
--
2.27.0
Thanks for the discussion.
At Tue, 13 Jul 2021 09:28:30 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Fri, Jul 09, 2021 at 04:50:28PM +0900, Kyotaro Horiguchi wrote:
> > At Fri, 9 Jul 2021 10:29:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> >> Er, wait. We've actually allowed negative values for pg_ctl
> >> --timeout or the subcommand kill!?
> >
> > --timeout accepts values less than 1, which values cause the command
> > ends with the timeout error before checking for the ending state. Not
> > destructive but useless as a behavior. We have two choices here. One
> > is simply inhibiting zero or negative timeouts, and another is
> > allowing zero as timeout and giving it the same meaning to
> > --no-wait. The former is strictly right behavior but the latter is
> > casual and convenient. I took the former way in this version.
>
> Yeah, that's not useful.
^^; Ok, I'm fine with taking the second way. Changed it.
"-t 0" means "-W" in the attached and that behavior is described in
the doc part. The environment variable PGCTLTIMEOUT accepts the same
range of values. I added a warning that notifies that -t 0 overrides
explicit -w .
> $ pg_ctl -w -t 0 start
> pg_ctl: WARNING: -w is ignored because timeout is set to 0
> server starting
> >> This one in pg_upgrade is incomplete. Perhaps the missing comment
> >> should tell that negative job values are checked later on?
> >
> > Zero or negative job numbers mean non-parallel mode and don't lead to
> > an error. If we don't need to preserve that behavior (I personally
> > don't think it is ether useful and/or breaks someone's existing
> > usage.), it is sensible to do range-check here.
>
> Hmm. It would be good to preserve some compatibility here, but I can
> see more benefits in being consistent between all the tools, and make
> people aware that such commands are not generated more carefully.
Ageed.
> > case 'j':
> > - opts.jobs = atoi(optarg);
> > - if (opts.jobs < 1)
> > + errno = 0;
> > + opts.jobs = strtoint(optarg, &endptr, 10);
> > + if (*endptr || errno == ERANGE || opts.jobs < 1)
> > {
> > pg_log_error("number of parallel jobs must be at least 1");
> > exit(1);
>
> specifying a value that triggers ERANGE could be confusing for values
> higher than INT_MAX with pg_amcheck --jobs:
> $ pg_amcheck --jobs=4000000000
> pg_amcheck: error: number of parallel jobs must be at least 1
> I think that this should be reworded as "invalid number of parallel
> jobs \"$s\"", or "number of parallel jobs must be in range %d..%d".
> Perhaps we could have a combination of both? Using the first message
> is useful with junk, non-numeric values or trailing characters. The
> second is useful to make users aware that the value is numeric, but
> not quite right.
Yeah, I noticed that but ignored as a kind of impossible, or
something-needless-to-say:p
> "invalid number of parallel jobs \"$s\""
> "number of parallel jobs must be in range %d..%d"
The resulting combined message looks like this:
> "number of parallel jobs must be an integer in range 1..2147483647"
I don't think it's not great that the message explicitly suggests a
limit like INT_MAX, which is far above the practical limit. So, (even
though I avoided to do that but) in the attached, I changed my mind to
split most of the errors into two messages to avoid suggesting such
impractical limits.
$ pg_amcheck -j -1
$ pg_amcheck -j 1x
$ pg_amcheck -j 10000000000000x
pg_amcheck: error: number of parallel jobs must be an integer greater than zero: "...."
$ pg_amcheck -j 10000000000000
pg_amcheck: error: number of parallel jobs out of range: "10000000000000"
If you feel it's too-much or pointless to split that way, I'll happy
to change it the "%d..%d" form.
Still I used the "%d..%d" notation for some parameters that has a
finite range by design. They look like the following:
(%d..%d doesn't work well for real numbers.)
"sampling rate must be an real number between 0.0 and 1.0: \"%s\""
I'm not sure what to do for numWorkers of pg_dump/restore. The limit
for numWorkers are lowered on Windows to quite low value, which would
be worth showing, but otherwise the limit is INT_MAX. The attached
makes pg_dump respond to -j 100 with the following error message which
doesn't suggest the finite limit 64 on Windows.
$ pg_dump -j 100
pg_dump: error: number of parallel jobs out of range: "100"
> > @@ -587,8 +602,10 @@ main(int argc, char **argv)
> >
> > case 8:
> > have_extra_float_digits = true;
> > - extra_float_digits = atoi(optarg);
> > - if (extra_float_digits < -15 || extra_float_digits > 3)
> > + errno = 0;
> > + extra_float_digits = strtoint(optarg, &endptr, 10);
> > + if (*endptr || errno == ERANGE ||
> > + extra_float_digits < -15 || extra_float_digits > 3)
> > {
> > pg_log_error("extra_float_digits must be in range -15..3");
> > exit_nicely(1);
>
> Should we take this occasion to reduce the burden of translators and
> reword that as "%s must be in range %d..%d"? That could be a separate
> patch.
The first %s is not always an invariable symbol name so it could
result in concatenating several phrases into one, like this.
pg_log..("%s must be in range %s..%s", _("compression level"), "0", "9"))
It is translatable at least into Japanese but I'm not sure about other
languages.
In the attached, most of the messages are not in this shape since I
avoided to suggest substantially infinite limits, thus this doesn't
fully work. I'll consider it if the current shape is found to be
unacceptable. In that case I'll use the option names in the messages
instead of the natural names.
> > case 'p':
> > - if ((old_cluster.port = atoi(optarg)) <= 0)
> > - pg_fatal("invalid old port number\n");
> > + errno = 0;
> > + if ((old_cluster.port = strtoint(optarg, &endptr, 10)) <= 0 ||
> > + *endptr || errno == ERANGE)
> > + pg_fatal("invalid old port number %s\n", optarg);
> > break;
>
> You may want to use columns here, or specify the port range:
> "invalid old port number: %s" or "old port number must be in range
> %d..%d".
I'm not sure whether the colons(?) are required, since pg_receivewal
currently complains as "invalid port number \"%s\"" but I agree that
it would be better if we had one.
By the way, in the attached version, the message is split into the
following combination. ("an integer" might be useless..)
pg_fatal("old port number must be an integer greater than zero: \"%s\"\n",
pg_fatal("old port number out of range: \"%s\"\n", optarg);
As the result.
> > + if (*endptr || errno == ERANGE || concurrentCons <= 0)
> > {
> > - pg_log_error("number of parallel jobs must be at least 1");
> > + pg_log_error("number of parallel jobs must be at least 1: %s", optarg);
> > exit(1);
> > }
>
> This one is also confusing with optorg > INT_MAX.
The attached version says just "out of range" in that case.
- Is it worth avoiding suggesting a virtually infinite upper limit by
splitting out "out of range" from an error message?
- If not, I'll use the single message "xxx must be in range
1..2147483647" or "xxx must be an integer in range 1..2147483647".
Do you think we need the parameter type "an integer" there?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From 56d4fda06a9003efe1172848180114e42066db98 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 15:08:01 +0900
Subject: [PATCH v3 1/3] Be strict in numeric parameters on command line
Some numeric command line parameters are tolerant of valid values
followed by garbage like "123xyz". Be strict to reject such invalid
values. Do the same for psql meta command parameters.
---
src/bin/pg_amcheck/pg_amcheck.c | 15 ++-
src/bin/pg_basebackup/pg_basebackup.c | 24 +++-
src/bin/pg_basebackup/pg_receivewal.c | 39 +++++--
src/bin/pg_basebackup/pg_recvlogical.c | 44 +++++--
src/bin/pg_checksums/pg_checksums.c | 17 ++-
src/bin/pg_ctl/pg_ctl.c | 42 ++++++-
src/bin/pg_dump/pg_dump.c | 57 ++++++---
src/bin/pg_dump/pg_restore.c | 42 ++++---
src/bin/pg_upgrade/option.c | 30 ++++-
src/bin/pgbench/pgbench.c | 154 +++++++++++++++++++------
src/bin/psql/command.c | 73 +++++++++++-
src/bin/scripts/reindexdb.c | 15 ++-
src/bin/scripts/vacuumdb.c | 59 ++++++++--
13 files changed, 484 insertions(+), 127 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..f40d58ac96 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -17,6 +17,7 @@
#include "catalog/pg_am_d.h"
#include "catalog/pg_namespace_d.h"
#include "common/logging.h"
+#include "common/string.h"
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
@@ -326,10 +327,18 @@ main(int argc, char *argv[])
append_btree_pattern(&opts.exclude, optarg, encoding);
break;
case 'j':
- opts.jobs = atoi(optarg);
- if (opts.jobs < 1)
+ errno = 0;
+ opts.jobs = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("number of parallel jobs must be at least 1");
+ pg_log_error("number of parallel jobs out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || opts.jobs < 1)
+ {
+ pg_log_error("number of parallel jobs must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf498..c30005f569 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2287,6 +2287,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
long_options, &option_index)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'C':
@@ -2371,10 +2373,12 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ errno = 0;
+ compresslevel = strtoint(optarg, &endptr, 10);
+ if (*endptr ||
+ errno == ERANGE || compresslevel < 0 || compresslevel > 9)
{
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("compression level must be a digit in range 0..9: \"%s\"", optarg);
exit(1);
}
break;
@@ -2409,10 +2413,18 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+ pg_log_error("status interval out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || standby_message_timeout < 0)
+ {
+ pg_log_error("status interval must be a non-negative integer: \"%s\"",
+ optarg);
exit(1);
}
break;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index c1334fad35..fb03147fe7 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -22,6 +22,7 @@
#include "access/xlog_internal.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "receivelog.h"
@@ -520,6 +521,9 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:",
long_options, &option_index)) != -1)
{
+ char *endptr;
+ int v;
+
switch (c)
{
case 'D':
@@ -532,9 +536,17 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ errno = 0;
+ v = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid port number \"%s\"", optarg);
+ pg_log_error("port number out of range: \"%s\"", optarg);
+ exit(1);
+ }
+ if (*endptr || v < 1)
+ {
+ pg_log_error("port number must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
dbport = pg_strdup(optarg);
@@ -549,10 +561,18 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+ pg_log_error("status interval out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || standby_message_timeout < 0)
+ {
+ pg_log_error("status interval must be a non-negative integer: \"%s\"",
+ optarg);
exit(1);
}
break;
@@ -574,10 +594,13 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ errno = 0;
+ compresslevel = strtoint(optarg, &endptr, 10);
+ if (*endptr ||
+ errno == ERANGE || compresslevel < 0 || compresslevel > 9)
{
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("compression level must be a digit in range 0..9: \"%s\"",
+ optarg);
exit(1);
}
break;
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 76bd153fac..9bc4902033 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -23,6 +23,7 @@
#include "common/fe_memutils.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -732,6 +733,9 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "E:f:F:nvtd:h:p:U:wWI:o:P:s:S:",
long_options, &option_index)) != -1)
{
+ char *endptr;
+ int v;
+
switch (c)
{
/* general options */
@@ -739,10 +743,18 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ errno = 0;
+ fsync_interval = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid fsync interval \"%s\"", optarg);
+ pg_log_error("fsync interval out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || fsync_interval < 0)
+ {
+ pg_log_error("fsync interval must be a non-negative integer: \"%s\"",
+ optarg);
exit(1);
}
break;
@@ -763,9 +775,17 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ errno = 0;
+ v = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid port number \"%s\"", optarg);
+ pg_log_error("port number out of range: \"%s\"", optarg);
+ exit(1);
+ }
+ if (*endptr || v < 1)
+ {
+ pg_log_error("port number must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
dbport = pg_strdup(optarg);
@@ -820,10 +840,18 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ errno = 0;
+ standby_message_timeout = strtoint(optarg, &endptr, 10) * 1000;
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+ pg_log_error("status interval out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || standby_message_timeout < 0)
+ {
+ pg_log_error("status interval must be a non-negative integer: \"%s\"",
+ optarg);
exit(1);
}
break;
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3c326906e2..78a1d4ef38 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -24,6 +24,7 @@
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
+#include "common/string.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -506,6 +507,9 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "cD:deNPf:v", long_options, &option_index)) != -1)
{
+ char *endptr;
+ int v;
+
switch (c)
{
case 'c':
@@ -518,9 +522,18 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
case 'f':
- if (atoi(optarg) == 0)
+ errno = 0;
+ v = strtoint(optarg, &endptr, 10);
+ if(*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
+ pg_log_error("filenode specification out of range: %s",
+ optarg);
+ exit(1);
+ }
+ if(*endptr || v < 1)
+ {
+ pg_log_error("filenode specification must be an integer greater than zero: %s",
+ optarg);
exit(1);
}
only_filenode = pstrdup(optarg);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..0f72ef016b 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -76,6 +76,7 @@ typedef enum
#define WAITS_PER_SEC 10 /* should divide USEC_PER_SEC evenly */
+static bool do_wait_arg = false;
static bool do_wait = true;
static int wait_seconds = DEFAULT_WAIT;
static bool wait_seconds_arg = false;
@@ -2331,6 +2332,8 @@ main(int argc, char **argv)
/* process command-line options */
while (optind < argc)
{
+ char *endptr;
+
while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
long_options, &option_index)) != -1)
{
@@ -2396,7 +2399,20 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ errno = 0;
+ wait_seconds = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ {
+ pg_log_error("timeout value out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || wait_seconds < 0)
+ {
+ pg_log_error("timeout value must be a non-negative integer: \"%s\"",
+ optarg);
+ exit(1);
+ }
wait_seconds_arg = true;
break;
case 'U':
@@ -2408,6 +2424,7 @@ main(int argc, char **argv)
break;
case 'w':
do_wait = true;
+ do_wait_arg = true;
break;
case 'W':
do_wait = false;
@@ -2459,7 +2476,20 @@ main(int argc, char **argv)
}
ctl_command = KILL_COMMAND;
set_sig(argv[++optind]);
- killproc = atol(argv[++optind]);
+ errno = 0;
+ killproc = strtol(argv[++optind], &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ {
+ pg_log_error("process ID out of range: \"%s\"",
+ argv[optind]);
+ exit(1);
+ }
+ if (*endptr || killproc < 0)
+ {
+ pg_log_error("process ID must be a non-negative integer: \"%s\"",
+ argv[optind]);
+ exit(1);
+ }
}
#ifdef WIN32
else if (strcmp(argv[optind], "register") == 0)
@@ -2514,6 +2544,14 @@ main(int argc, char **argv)
do_wait = false;
}
+ if (wait_seconds == 0 && do_wait)
+ {
+ /* Warn if user instructed to wait but we actually don't */
+ if (!silent_mode && do_wait_arg)
+ write_stderr(_("%s: WARNING: -w is ignored because timeout is set to 0\n"), progname);
+ do_wait = false;
+ }
+
if (pg_data)
{
snprintf(postopts_file, MAXPGPATH, "%s/postmaster.opts", pg_data);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 321152151d..8ef29b37f6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -54,6 +54,7 @@
#include "catalog/pg_trigger_d.h"
#include "catalog/pg_type_d.h"
#include "common/connect.h"
+#include "common/string.h"
#include "dumputils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
@@ -103,6 +104,17 @@ static Oid g_last_builtin_oid; /* value of the last builtin oid */
/* The specified names/patterns should to match at least one entity */
static int strict_names = 0;
+/*
+ * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
+ * parallel jobs because that's the maximum limit for the
+ * WaitForMultipleObjects() call.
+ */
+#ifndef WIN32
+#define MAX_NUM_WORKERS INT_MAX
+#else
+#define MAX_NUM_WORKERS MAXIMUM_WAIT_OBJECTS
+#endif
+
/*
* Object inclusion/exclusion lists
*
@@ -486,7 +498,21 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ errno = 0;
+ numWorkers = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 &&
+ (errno == ERANGE || numWorkers > MAX_NUM_WORKERS))
+ {
+ pg_log_error("number of parallel jobs out of range: \"%s\"",
+ optarg);
+ exit_nicely(1);
+ }
+ if (*endptr || numWorkers <= 0)
+ {
+ pg_log_error("number of parallel jobs must be an integer greater than zero: \"%s\"",
+ optarg);
+ exit_nicely(1);
+ }
break;
case 'n': /* include schema(s) */
@@ -549,10 +575,12 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ errno = 0;
+ compressLevel = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ compressLevel < 0 || compressLevel > 9)
{
- pg_log_error("compression level must be in range 0..9");
+ pg_log_error("compression level must be a digit in range 0..9: \"%s\"", optarg);
exit_nicely(1);
}
break;
@@ -587,10 +615,13 @@ main(int argc, char **argv)
case 8:
have_extra_float_digits = true;
- extra_float_digits = atoi(optarg);
- if (extra_float_digits < -15 || extra_float_digits > 3)
+ errno = 0;
+ extra_float_digits = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ extra_float_digits < -15 || extra_float_digits > 3)
{
- pg_log_error("extra_float_digits must be in range -15..3");
+ pg_log_error("extra_float_digits must be an integer in range -15..3: \"%s\"",
+ optarg);
exit_nicely(1);
}
break;
@@ -719,18 +750,6 @@ main(int argc, char **argv)
if (!plainText)
dopt.outputCreateDB = 1;
- /*
- * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
- * parallel jobs because that's the maximum limit for the
- * WaitForMultipleObjects() call.
- */
- if (numWorkers <= 0
-#ifdef WIN32
- || numWorkers > MAXIMUM_WAIT_OBJECTS
-#endif
- )
- fatal("invalid number of parallel jobs");
-
/* Parallel backup only in the directory archive format so far */
if (archiveFormat != archDirectory && numWorkers > 1)
fatal("parallel backup only supported by the directory format");
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 589b4aed53..3bb5a48c55 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -39,6 +39,7 @@
*-------------------------------------------------------------------------
*/
#include "postgres_fe.h"
+#include "common/string.h"
#include <ctype.h>
#ifdef HAVE_TERMIOS_H
@@ -52,6 +53,13 @@
static void usage(const char *progname);
+/* See comments in pg_dump.c */
+#ifndef WIN32
+#define MAX_NUM_WORKERS INT_MAX
+#else
+#define MAX_NUM_WORKERS MAXIMUM_WAIT_OBJECTS
+#endif
+
int
main(int argc, char **argv)
{
@@ -151,6 +159,8 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
cmdopts, NULL)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'a': /* Dump data only */
@@ -181,7 +191,21 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ errno = 0;
+ numWorkers = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 &&
+ (errno == ERANGE || numWorkers > MAX_NUM_WORKERS))
+ {
+ pg_log_error("number of parallel jobs out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || numWorkers <= 0)
+ {
+ pg_log_error("number of parallel jobs must be an integer greater than zero: \"%s\"",
+ optarg);
+ exit(1);
+ }
break;
case 'l': /* Dump the TOC summary */
@@ -344,22 +368,6 @@ main(int argc, char **argv)
exit_nicely(1);
}
- if (numWorkers <= 0)
- {
- pg_log_error("invalid number of parallel jobs");
- exit(1);
- }
-
- /* See comments in pg_dump.c */
-#ifdef WIN32
- if (numWorkers > MAXIMUM_WAIT_OBJECTS)
- {
- pg_log_error("maximum number of parallel jobs is %d",
- MAXIMUM_WAIT_OBJECTS);
- exit(1);
- }
-#endif
-
/* Can't do single-txn mode with multiple connections */
if (opts->single_txn && numWorkers > 1)
{
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 64bbda5650..c014bbca0d 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -104,6 +104,8 @@ parseCommandLine(int argc, char *argv[])
while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (option)
{
case 'b':
@@ -127,7 +129,15 @@ parseCommandLine(int argc, char *argv[])
break;
case 'j':
- user_opts.jobs = atoi(optarg);
+ errno = 0;
+ user_opts.jobs = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ pg_fatal("number of parallel jobs out of range: \"%s\"\n",
+ optarg);
+ if (*endptr || user_opts.jobs < 1)
+ pg_fatal("number of parallel jobs must be an integer greater than zero: \"%s\"\n",
+ optarg);
+
break;
case 'k':
@@ -166,13 +176,23 @@ parseCommandLine(int argc, char *argv[])
* supported on all old/new versions (added in PG 9.2).
*/
case 'p':
- if ((old_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid old port number\n");
+ errno = 0;
+ old_cluster.port = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ pg_fatal("old port number out of range: \"%s\"\n", optarg);
+ if (*endptr || old_cluster.port <= 0)
+ pg_fatal("old port number must be an integer greater than zero: \"%s\"\n",
+ optarg);
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid new port number\n");
+ errno = 0;
+ new_cluster.port = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ pg_fatal("new port number out of range: \"%s\"\n", optarg);
+ if (*endptr || new_cluster.port <= 0)
+ pg_fatal("new port number must be an integer greater than zero: \"%s\"\n",
+ optarg);
break;
case 'r':
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..1c3b5836c1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5856,6 +5856,7 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) !=
-1)
{
char *script;
+ char *endptr;
switch (c)
{
@@ -5887,10 +5888,18 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
+ errno = 0;
+ nclients = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid number of clients: \"%s\"", optarg);
+ pg_log_fatal("number of clients out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || nclients <= 0)
+ {
+ pg_log_fatal("number of clients must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
#ifdef HAVE_GETRLIMIT
@@ -5914,10 +5923,18 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ errno = 0;
+ nthreads = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid number of threads: \"%s\"", optarg);
+ pg_log_fatal("number of threads out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || nthreads <= 0)
+ {
+ pg_log_fatal("number of threads must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
#ifndef ENABLE_THREAD_SAFETY
@@ -5938,28 +5955,50 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ errno = 0;
+ scale = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid scaling factor: \"%s\"", optarg);
+ pg_log_fatal("scaling factor out of range: \"%s\"", optarg);
+ exit(1);
+ }
+ if (*endptr || scale <= 0)
+ {
+ pg_log_fatal("scaling factor must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ errno = 0;
+ nxacts = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid number of transactions: \"%s\"", optarg);
+ pg_log_fatal("number of transactions out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || nxacts <= 0)
+ {
+ pg_log_fatal("number of transactions must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
+ errno = 0;
+ duration = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid duration: \"%s\"", optarg);
+ pg_log_fatal("duration out of range: \"%s\"", optarg);
+ exit(1);
+ }
+ if (*endptr || duration <= 0)
+ {
+ pg_log_fatal("duration must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
@@ -6019,10 +6058,13 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ errno = 0;
+ fillfactor = strtoint(optarg, &endptr, 10);
+ if (*endptr || errno == ERANGE ||
+ fillfactor < 10 || fillfactor > 100)
{
- pg_log_fatal("invalid fillfactor: \"%s\"", optarg);
+ pg_log_fatal("fillfactor must be an ineger between 10 and 100: \"%s\"",
+ optarg);
exit(1);
}
break;
@@ -6039,23 +6081,38 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ errno = 0;
+ progress = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid thread progress delay: \"%s\"", optarg);
+ pg_log_fatal("thread progress delay out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || progress <= 0)
+ {
+ pg_log_fatal("thread progress delay must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
case 'R':
{
/* get a double from the beginning of option value */
- double throttle_value = atof(optarg);
+ double throttle_value;
+ errno = 0;
+ throttle_value = strtod(optarg, &endptr);
benchmarking_option_set = true;
- if (throttle_value <= 0.0)
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid rate limit: \"%s\"", optarg);
+ pg_log_fatal("rate limit out of range: \"%s\"", optarg);
+ exit(1);
+ }
+ if (*endptr || throttle_value <= 0.0)
+ {
+ pg_log_fatal("rate limit must be a real number greater than zero: \"%s\"", optarg);
exit(1);
}
/* Invert rate limit into per-transaction delay in usec */
@@ -6064,11 +6121,20 @@ main(int argc, char **argv)
break;
case 'L':
{
- double limit_ms = atof(optarg);
+ double limit_ms;
- if (limit_ms <= 0.0)
+ errno = 0;
+ limit_ms = strtod(optarg, &endptr);
+
+ if (*endptr == 0 && errno == ERANGE)
+ {
+ pg_log_fatal("latency limit out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || limit_ms <= 0.0)
{
- pg_log_fatal("invalid latency limit: \"%s\"", optarg);
+ pg_log_fatal("latency limit must be a real number greater than zero: \"%s\"", optarg);
exit(1);
}
benchmarking_option_set = true;
@@ -6089,19 +6155,27 @@ main(int argc, char **argv)
break;
case 4: /* sampling-rate */
benchmarking_option_set = true;
- sample_rate = atof(optarg);
- if (sample_rate <= 0.0 || sample_rate > 1.0)
+ errno = 0;
+ sample_rate = strtod(optarg, &endptr);
+ if (*endptr || errno == ERANGE ||
+ sample_rate <= 0.0 || sample_rate > 1.0)
{
- pg_log_fatal("invalid sampling rate: \"%s\"", optarg);
+ pg_log_fatal("sampling rate must be an real number between 0.0 and 1.0: \"%s\"", optarg);
exit(1);
}
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ errno = 0;
+ agg_interval = strtod(optarg, &endptr);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg);
+ pg_log_fatal("aggregate interval out of range: \"%s\"", optarg);
+ exit(1);
+ }
+ if (*endptr || agg_interval <= 0)
+ {
+ pg_log_fatal("aggregate interval must be a real number greater than zero: \"%s\"", optarg);
exit(1);
}
break;
@@ -6135,10 +6209,18 @@ main(int argc, char **argv)
break;
case 11: /* partitions */
initialization_option_set = true;
- partitions = atoi(optarg);
- if (partitions < 0)
+ errno = 0;
+ partitions = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_fatal("invalid number of partitions: \"%s\"", optarg);
+ pg_log_fatal("number of partitions out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || partitions < 0)
+ {
+ pg_log_fatal("number of partitions must be a non-negative integer: \"%s\"",
+ optarg);
exit(1);
}
break;
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index d704c4220c..13074051b3 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1040,10 +1040,18 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (ln)
{
- lineno = atoi(ln);
- if (lineno < 1)
+ char *endptr;
+
+ errno = 0;
+ lineno = strtoint(ln, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("invalid line number: %s", ln);
+ pg_log_error("line number out of range: %s", ln);
+ status = PSQL_CMD_ERROR;
+ }
+ if (*endptr || lineno < 1)
+ {
+ pg_log_error("line number must be an integer greater than zero: %s", ln);
status = PSQL_CMD_ERROR;
}
}
@@ -4284,7 +4292,25 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "border") == 0)
{
if (value)
- popt->topt.border = atoi(value);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr == 0 && (errno == ERANGE || new_value > 65535))
+ {
+ pg_log_error("\\pset: border out of range");
+ return false;
+ }
+ if (*endptr || new_value < 0)
+ {
+ pg_log_error("\\pset: border must be an integer greater than zero");
+ return false;
+ }
+
+ popt->topt.border = new_value;
+ }
}
/* set expanded/vertical mode */
@@ -4440,7 +4466,25 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "pager_min_lines") == 0)
{
if (value)
- popt->topt.pager_min_lines = atoi(value);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ {
+ pg_log_error("\\pset: pager_min_lines out of range");
+ return false;
+ }
+ if (*endptr || new_value < 0)
+ {
+ pg_log_error("\\pset: pager_min_lines must be a non-negative integer");
+ return false;
+ }
+
+ popt->topt.pager_min_lines = new_value;
+ }
}
/* disable "(x rows)" footer */
@@ -4456,7 +4500,24 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "columns") == 0)
{
if (value)
- popt->topt.columns = atoi(value);
+ {
+ char *endptr;
+ int new_value;
+
+ errno = 0;
+ new_value = strtoint(value, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
+ {
+ pg_log_error("\\pset: column out of range");
+ return false;
+ }
+ if (*endptr || new_value < 0)
+ {
+ pg_log_error("\\pset: column must be a non-negative integer");
+ return false;
+ }
+ popt->topt.columns = new_value;
+ }
}
else
{
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc0681538a..42d4d20768 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -15,6 +15,7 @@
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
#include "fe_utils/parallel_slot.h"
@@ -109,6 +110,8 @@ main(int argc, char *argv[])
/* process command-line options */
while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'h':
@@ -151,10 +154,16 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ errno = 0;
+ concurrentCons = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("number of parallel jobs must be at least 1");
+ pg_log_error("number of parallel jobs out of range: %s", optarg);
+ exit(1);
+ }
+ if (*endptr || concurrentCons <= 0)
+ {
+ pg_log_error("number of parallel jobs must be an integer greater than zero: %s", optarg);
exit(1);
}
break;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 61974baa78..6b2a34edd0 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -17,6 +17,7 @@
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
#include "fe_utils/parallel_slot.h"
@@ -141,6 +142,8 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:P:", long_options, &optindex)) != -1)
{
+ char *endptr;
+
switch (c)
{
case 'h':
@@ -192,18 +195,34 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ errno = 0;
+ concurrentCons = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("number of parallel jobs must be at least 1");
+ pg_log_error("number of parallel jobs out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || concurrentCons <= 0)
+ {
+ pg_log_error("number of parallel jobs must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
case 'P':
- vacopts.parallel_workers = atoi(optarg);
- if (vacopts.parallel_workers < 0)
+ errno = 0;
+ vacopts.parallel_workers = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("parallel workers for vacuum must be greater than or equal to zero");
+ pg_log_error("parallel workers out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || vacopts.parallel_workers < 0)
+ {
+ pg_log_error("parallel workers for vacuum must be a non-negative integer: \"%s\"",
+ optarg);
exit(1);
}
break;
@@ -220,18 +239,34 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
+ errno = 0;
+ vacopts.min_xid_age = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("minimum transaction ID age must be at least 1");
+ pg_log_error("minimum transaction ID age out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || vacopts.min_xid_age <= 0)
+ {
+ pg_log_error("minimum transaction ID age must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
+ errno = 0;
+ vacopts.min_mxid_age = strtoint(optarg, &endptr, 10);
+ if (*endptr == 0 && errno == ERANGE)
{
- pg_log_error("minimum multixact ID age must be at least 1");
+ pg_log_error("minimum multixact ID age out of range: \"%s\"",
+ optarg);
+ exit(1);
+ }
+ if (*endptr || vacopts.min_mxid_age <= 0)
+ {
+ pg_log_error("minimum multixact ID age must be an integer greater than zero: \"%s\"",
+ optarg);
exit(1);
}
break;
--
2.27.0
From 640b6b63ddc1c79b1013ba7f4956f8a0d62c40ae Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 17:10:19 +0900
Subject: [PATCH v3 2/3] Make complain for invalid numeirc values in
environemnt variables
Postgresql commands that read environment variables for integer values
are tolerant of trailing garbages like '5432xyz'. Ignore such values
and make warning on it.
---
src/bin/pg_ctl/pg_ctl.c | 12 +++++++++++-
src/bin/pg_upgrade/option.c | 32 ++++++++++++++++++++++++++++++--
src/bin/psql/startup.c | 17 ++++++++++++++++-
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 0f72ef016b..0c5dba5ecf 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2319,7 +2319,17 @@ main(int argc, char **argv)
env_wait = getenv("PGCTLTIMEOUT");
if (env_wait != NULL)
- wait_seconds = atoi(env_wait);
+ {
+ char *endptr;
+ int tmp_wait;
+
+ errno = 0;
+ tmp_wait = strtoint(env_wait, &endptr, 10);
+ if (*endptr || errno == ERANGE || tmp_wait < 0)
+ write_stderr(_("%s: ignored invalid setting of environment variable PGCTLTIMEOUT: %s\n"), progname,
env_wait);
+ else
+ wait_seconds = tmp_wait;
+ }
/*
* 'Action' can be before or after args so loop over both. Some
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index c014bbca0d..0acb6b7cdc 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -71,8 +71,36 @@ parseCommandLine(int argc, char *argv[])
os_info.progname = get_progname(argv[0]);
/* Process libpq env. variables; load values here for usage() output */
- old_cluster.port = getenv("PGPORTOLD") ? atoi(getenv("PGPORTOLD")) : DEF_PGUPORT;
- new_cluster.port = getenv("PGPORTNEW") ? atoi(getenv("PGPORTNEW")) : DEF_PGUPORT;
+ old_cluster.port = new_cluster.port = DEF_PGUPORT;
+ if (getenv("PGPORTOLD"))
+ {
+ char *endptr;
+ int port;
+
+ errno = 0;
+ port = strtoint(getenv("PGPORTOLD"), &endptr, 10);
+ if (*endptr || errno == ERANGE || port < 1)
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTOLD: %s\n",
+ os_info.progname, getenv("PGPORTOLD"));
+ else
+ old_cluster.port = port;
+ }
+
+ if (getenv("PGPORTNEW"))
+ {
+ char *endptr;
+ int port;
+
+ errno = 0;
+ port = strtoint(getenv("PGPORTNEW"), &endptr, 10);
+ if (*endptr || errno == ERANGE || port < 1)
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTNEW: %s\n",
+ os_info.progname, getenv("PGPORTNEW"));
+ else
+ new_cluster.port = port;
+ }
os_user_effective_id = get_user_info(&os_info.user);
/* we override just the database user name; we got the OS id above */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5f36f0d1c6..35ccabce77 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -181,7 +181,22 @@ main(int argc, char *argv[])
refresh_utf8format(&(pset.popt.topt));
/* We must get COLUMNS here before readline() sets it */
- pset.popt.topt.env_columns = getenv("COLUMNS") ? atoi(getenv("COLUMNS")) : 0;
+ pset.popt.topt.env_columns = 0;
+ if (getenv("COLUMNS"))
+ {
+ char *endptr;
+ int cols;
+
+ errno = 0;
+ cols = strtoint(getenv("COLUMNS"), &endptr, 10);
+ if (*endptr || errno == ERANGE || cols < 0)
+ {
+ pg_log_warning("ignored invalid setting of environemt variable COLUMNS: %s",
+ getenv("COLUMNS"));
+ }
+
+ pset.popt.topt.env_columns = cols;
+ }
pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout)));
--
2.27.0
From 4760039c811d2242bd517f72a515bac416d94710 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 13 Jul 2021 19:35:12 +0900
Subject: [PATCH v3 3/3] Doc change for pg_ctl
The previous patch change the bahavior of pg_ctl about
--timeout. Change the doc following the behavior change.
---
doc/src/sgml/ref/pg_ctl-ref.sgml | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index 3946fa52ea..90e0fabc63 100644
--- a/doc/src/sgml/ref/pg_ctl-ref.sgml
+++ b/doc/src/sgml/ref/pg_ctl-ref.sgml
@@ -389,9 +389,11 @@ PostgreSQL documentation
<listitem>
<para>
Specifies the maximum number of seconds to wait when waiting for an
- operation to complete (see option <option>-w</option>). Defaults to
- the value of the <envar>PGCTLTIMEOUT</envar> environment variable or, if
- not set, to 60 seconds.
+ operation to complete (see option <option>-w</option>). If it is set
+ to zero, <command>pg_ctl</command> behaves the same way
+ with <option>-W</option>. Defaults to the value of
+ the <envar>PGCTLTIMEOUT</envar> environment variable or, if not set,
+ to 60 seconds.
</para>
</listitem>
</varlistentry>
--
2.27.0
At Thu, 15 Jul 2021 16:19:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Wed, Jul 14, 2021 at 11:02:47AM -0400, Alvaro Herrera wrote:
> > No, this doesn't work. When the first word is something that is
> > not to be translated (a literal parameter name), let's use a %s (but of
> > course the values must be taken out of the phrase too). But when it is
> > a translatable phrase, it must be present a full phrase, no
> > substitution:
> >
> > pg_log_error("%s must be in range %s..%s", "extra_float_digits", "-15", "3");
> > pg_log..("compression level must be in range %s..%s", "0", "9"))
> >
> > I think the purity test is whether you want to use a _() around the
> > argument, then you have to include it into the original message.
>
> After thinking about that, it seems to me that we don't have that much
> context to lose once we build those error messages to show the option
> name of the command. And the patch, as proposed, finishes with the
Agreed.
> same error message patterns all over the place, which would be a
> recipe for more inconsistencies in the future. I think that we should
> centralize all that, say with a small-ish routine in a new file called
> src/fe_utils/option_parsing.c that uses strtoint() as follows:
> bool option_parse_int(const char *optarg,
> const char *optname,
> int min_range,
> int max_range,
> int *result);
>
> Then this routine may print two types of errors through
> pg_log_error():
> - Incorrect range, using min_range/max_range.
> - Junk input.
> The boolean status is here to let the caller do any custom exit()
> actions he wishes if there one of those two failures. pg_dump has
> some of that with exit_nicely(), for one.
It is substantially the same suggestion with [1] in the original
thread. The original proposal in the old thread was
> bool pg_strtoint64_range(arg, &result, min, max, &error_message)
The difference is your suggestion is making the function output the
message within. I guess that the reason for the original proposal is
different style of message is required in several places.
Actually there are several irregulars.
1. Some "bare" options (that is not preceded by a hyphen option) like
PID of pg_ctl kill doesn't fit the format. \pset parameters of
pg_ctl is the same.
2. pg_ctl, pg_upgrade use their own error reporting mechanisms.
3. parameters that take real numbers doesn't fit the scheme specifying
range borders. For example boundary values may or may not be included
in the range.
4. Most of the errors are PG_LOG_ERROR, but a few ones are
PG_LOG_FATAL.
That being said, most of the caller sites are satisfied by
fixed-formed messages.
So I changed the interface as the following to deal with the above issues:
+extern optparse_result option_parse_int(int loglevel,
+ const char *optarg, const char *optname,
+ int min_range, int max_range,
+ int *result);
loglevel specifies the loglevel to use to emit error messages. If it
is the newly added item PG_LOG_OMIT, the function never emits an error
message. Addition to that, the return type is changed to an enum which
indicates what trouble the given string has. The caller can print
arbitrary error messages consulting the value. (killproc in pg_ctl.c)
Other points:
I added two more similar functions option_parse_long/double. The
former is a clone of _int. The latter doesn't perform explicit range
checks for the reason described above.
Maybe we need to make pg_upgraded use the common-logging features
instead of its own, but it is not included in this patch.
pgbench's -L option accepts out-of-range values for internal
variable. As the added comment says, we can limit the value with the
large exact number but I limited it to 3600s since I can't imagine
people needs larger latency limit than that.
Similarly to the above, -R option can take for example 1E-300, which
leads to int64 overflow later. Similar to -L, I don't think people
don't need less than 1E-5 or so as this parameter.
The attached patch needs more polish but should be enough to tell what
I have in my mind.
regards.
1: https://www.postgresql.org/message-id/CAKJS1f94kkuB_53LcEf0HF%2BuxMiTCk5FtLx9gSsXcCByUKMz1g%40mail.gmail.com
--
Kyotaro Horiguchi
NTT Open Source Software Center
From aaf81ac0340e9df3b74e18c1492e5984c0412fb5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 15:08:01 +0900
Subject: [PATCH v3 1/3] Be strict in numeric parameters on command line
Some numeric command line parameters are tolerant of valid values
followed by garbage like "123xyz". Be strict to reject such invalid
values. Do the same for psql meta command parameters.
---
src/bin/pg_amcheck/pg_amcheck.c | 9 +-
src/bin/pg_basebackup/pg_basebackup.c | 17 ++-
src/bin/pg_basebackup/pg_receivewal.c | 25 ++--
src/bin/pg_basebackup/pg_recvlogical.c | 30 ++---
src/bin/pg_checksums/Makefile | 4 +-
src/bin/pg_checksums/pg_checksums.c | 9 +-
src/bin/pg_ctl/Makefile | 4 +-
src/bin/pg_ctl/pg_ctl.c | 39 ++++++-
src/bin/pg_dump/pg_dump.c | 44 ++++---
src/bin/pg_dump/pg_restore.c | 29 ++---
src/bin/pg_upgrade/option.c | 24 +++-
src/bin/pgbench/pgbench.c | 119 ++++++++++---------
src/bin/psql/command.c | 63 ++++++++--
src/bin/scripts/reindexdb.c | 9 +-
src/bin/scripts/vacuumdb.c | 31 ++---
src/fe_utils/option_utils.c | 152 +++++++++++++++++++++++++
src/include/common/logging.h | 9 ++
src/include/fe_utils/option_utils.h | 23 ++++
18 files changed, 457 insertions(+), 183 deletions(-)
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4b..538436d4a5 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -12,11 +12,13 @@
*/
#include "postgres_fe.h"
+#include <limits.h>
#include <time.h>
#include "catalog/pg_am_d.h"
#include "catalog/pg_namespace_d.h"
#include "common/logging.h"
+#include "common/string.h"
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
@@ -326,12 +328,9 @@ main(int argc, char *argv[])
append_btree_pattern(&opts.exclude, optarg, encoding);
break;
case 'j':
- opts.jobs = atoi(optarg);
- if (opts.jobs < 1)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, INT_MAX, &opts.jobs))
exit(1);
- }
break;
case 'p':
port = pg_strdup(optarg);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8bb0acf498..42a65ffbc3 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -15,6 +15,7 @@
#include <unistd.h>
#include <dirent.h>
+#include <limits.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <signal.h>
@@ -31,6 +32,7 @@
#include "common/file_utils.h"
#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/recovery_gen.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
@@ -2371,12 +2373,9 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
- {
- pg_log_error("invalid compression level \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-Z/--compress",
+ 0, 9, &compresslevel))
exit(1);
- }
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2409,12 +2408,10 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-s/--status-interval",
+ 0, INT_MAX, &standby_message_timeout))
exit(1);
- }
break;
case 'v':
verbose++;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index c1334fad35..1f976ad3eb 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <signal.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -22,6 +23,8 @@
#include "access/xlog_internal.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "receivelog.h"
@@ -532,11 +535,10 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
- {
- pg_log_error("invalid port number \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-p/--port",
+ 1, 65535, NULL))
exit(1);
- }
+
dbport = pg_strdup(optarg);
break;
case 'U':
@@ -549,12 +551,10 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-s/--status-interval",
+ 0, INT_MAX, &standby_message_timeout))
exit(1);
- }
break;
case 'S':
replication_slot = pg_strdup(optarg);
@@ -574,12 +574,9 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
- {
- pg_log_error("invalid compression level \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-Z/--compress",
+ 0, 9, &compresslevel))
exit(1);
- }
break;
/* action */
case 1:
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 76bd153fac..44622c002b 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -13,6 +13,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <sys/stat.h>
#include <unistd.h>
#ifdef HAVE_SYS_SELECT_H
@@ -23,6 +24,8 @@
#include "common/fe_memutils.h"
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -739,12 +742,12 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
- {
- pg_log_error("invalid fsync interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-F/--fsync-interval",
+ 0, INT_MAX / 1000, &fsync_interval))
exit(1);
- }
+ /* convert to milliseconds */
+ fsync_interval *= 1000;
break;
case 'n':
noloop = 1;
@@ -763,11 +766,9 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
- {
- pg_log_error("invalid port number \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-p/--port",
+ 0, 65535, NULL))
exit(1);
- }
dbport = pg_strdup(optarg);
break;
case 'U':
@@ -820,12 +821,13 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
- {
- pg_log_error("invalid status interval \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "-s/--status-interval",
+ 0, INT_MAX / 1000,
+ &standby_message_timeout))
exit(1);
- }
+ /* convert to milliseconds */
+ standby_message_timeout *= 1000;
break;
case 'S':
replication_slot = pg_strdup(optarg);
diff --git a/src/bin/pg_checksums/Makefile b/src/bin/pg_checksums/Makefile
index ba62406105..a22cf107f9 100644
--- a/src/bin/pg_checksums/Makefile
+++ b/src/bin/pg_checksums/Makefile
@@ -15,13 +15,15 @@ subdir = src/bin/pg_checksums
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
OBJS = \
$(WIN32RES) \
pg_checksums.o
all: pg_checksums
-pg_checksums: $(OBJS) | submake-libpgport
+pg_checksums: $(OBJS) | submake-libpgport submake-libpgfeutils
$(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 3c326906e2..d62df9747a 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -15,6 +15,7 @@
#include "postgres_fe.h"
#include <dirent.h>
+#include <limits.h>
#include <time.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -24,6 +25,8 @@
#include "common/file_perm.h"
#include "common/file_utils.h"
#include "common/logging.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/bufpage.h"
@@ -518,11 +521,9 @@ main(int argc, char *argv[])
mode = PG_MODE_ENABLE;
break;
case 'f':
- if (atoi(optarg) == 0)
- {
- pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-f/--filenode",
+ 1, INT_MAX, NULL))
exit(1);
- }
only_filenode = pstrdup(optarg);
break;
case 'N':
diff --git a/src/bin/pg_ctl/Makefile b/src/bin/pg_ctl/Makefile
index 5d5f5372a3..ed03d6dc7d 100644
--- a/src/bin/pg_ctl/Makefile
+++ b/src/bin/pg_ctl/Makefile
@@ -24,13 +24,15 @@ LDFLAGS_INTERNAL += $(libpq_pgport)
SUBMAKE_LIBPQ := submake-libpq
endif
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
OBJS = \
$(WIN32RES) \
pg_ctl.o
all: pg_ctl
-pg_ctl: $(OBJS) | submake-libpgport $(SUBMAKE_LIBPQ)
+pg_ctl: $(OBJS) | submake-libpgport $(SUBMAKE_LIBPQ) submake-libpgfeutils
$(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
install: all installdirs
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7985da0a94..f7ccdcdc96 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -12,6 +12,7 @@
#include "postgres_fe.h"
#include <fcntl.h>
+#include <limits.h>
#include <signal.h>
#include <time.h>
#include <sys/stat.h>
@@ -28,6 +29,7 @@
#include "common/file_perm.h"
#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "utils/pidfile.h"
@@ -76,6 +78,7 @@ typedef enum
#define WAITS_PER_SEC 10 /* should divide USEC_PER_SEC evenly */
+static bool do_wait_arg = false;
static bool do_wait = true;
static int wait_seconds = DEFAULT_WAIT;
static bool wait_seconds_arg = false;
@@ -2396,7 +2399,9 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-t/--timeout",
+ 0, INT_MAX, &wait_seconds))
+ exit(1);
wait_seconds_arg = true;
break;
case 'U':
@@ -2408,6 +2413,7 @@ main(int argc, char **argv)
break;
case 'w':
do_wait = true;
+ do_wait_arg = true;
break;
case 'W':
do_wait = false;
@@ -2459,7 +2465,28 @@ main(int argc, char **argv)
}
ctl_command = KILL_COMMAND;
set_sig(argv[++optind]);
- killproc = atol(argv[++optind]);
+ switch (option_parse_long(PG_LOG_OMIT, argv[++optind], "PID",
+ 0, LONG_MAX, &killproc))
+ {
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ write_stderr(_("%s: PID must be an integer: \"%s\"\n"),
+ progname, argv[optind]);
+ exit(1);
+ break;
+ case OPTPARSE_TOOSMALL:
+ write_stderr(_("%s: PID must be a non-negative integer: %s\n"),
+ progname, argv[optind]);
+ exit(1);
+ break;
+ case OPTPARSE_TOOLARGE:
+ case OPTPARSE_OUTOFRANGE:
+ write_stderr(_("%s: PID out of range: %s\n"),
+ progname, argv[optind]);
+ exit(1);
+ break;
+ }
}
#ifdef WIN32
else if (strcmp(argv[optind], "register") == 0)
@@ -2514,6 +2541,14 @@ main(int argc, char **argv)
do_wait = false;
}
+ if (wait_seconds == 0 && do_wait)
+ {
+ /* Warn if user instructed to wait but we actually don't */
+ if (!silent_mode && do_wait_arg)
+ write_stderr(_("%s: WARNING: -w is ignored because timeout is set to 0\n"), progname);
+ do_wait = false;
+ }
+
if (pg_data)
{
snprintf(postopts_file, MAXPGPATH, "%s/postmaster.opts", pg_data);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 34b91bb226..41b360edc0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -55,7 +55,9 @@
#include "catalog/pg_trigger_d.h"
#include "catalog/pg_type_d.h"
#include "common/connect.h"
+#include "common/string.h"
#include "dumputils.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq/libpq-fs.h"
@@ -104,6 +106,17 @@ static Oid g_last_builtin_oid; /* value of the last builtin oid */
/* The specified names/patterns should to match at least one entity */
static int strict_names = 0;
+/*
+ * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
+ * parallel jobs because that's the maximum limit for the
+ * WaitForMultipleObjects() call.
+ */
+#ifndef WIN32
+#define MAX_NUM_WORKERS INT_MAX
+#else
+#define MAX_NUM_WORKERS MAXIMUM_WAIT_OBJECTS
+#endif
+
/*
* Object inclusion/exclusion lists
*
@@ -487,7 +500,9 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, MAX_NUM_WORKERS, &numWorkers))
+ exit_nicely(1);
break;
case 'n': /* include schema(s) */
@@ -550,12 +565,9 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
- {
- pg_log_error("compression level must be in range 0..9");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-Z/--compress",
+ 0, 9, &compressLevel))
exit_nicely(1);
- }
break;
case 0:
@@ -588,12 +600,10 @@ main(int argc, char **argv)
case 8:
have_extra_float_digits = true;
- extra_float_digits = atoi(optarg);
- if (extra_float_digits < -15 || extra_float_digits > 3)
- {
- pg_log_error("extra_float_digits must be in range -15..3");
+ if (option_parse_int(PG_LOG_ERROR, optarg,
+ "--extra-float-digits",
+ -15, 3, &extra_float_digits))
exit_nicely(1);
- }
break;
case 9: /* inserts */
@@ -720,18 +730,6 @@ main(int argc, char **argv)
if (!plainText)
dopt.outputCreateDB = 1;
- /*
- * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
- * parallel jobs because that's the maximum limit for the
- * WaitForMultipleObjects() call.
- */
- if (numWorkers <= 0
-#ifdef WIN32
- || numWorkers > MAXIMUM_WAIT_OBJECTS
-#endif
- )
- fatal("invalid number of parallel jobs");
-
/* Parallel backup only in the directory archive format so far */
if (archiveFormat != archDirectory && numWorkers > 1)
fatal("parallel backup only supported by the directory format");
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 589b4aed53..6f4447027a 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -39,6 +39,8 @@
*-------------------------------------------------------------------------
*/
#include "postgres_fe.h"
+#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include <ctype.h>
#ifdef HAVE_TERMIOS_H
@@ -52,6 +54,13 @@
static void usage(const char *progname);
+/* See comments in pg_dump.c */
+#ifndef WIN32
+#define MAX_NUM_WORKERS INT_MAX
+#else
+#define MAX_NUM_WORKERS MAXIMUM_WAIT_OBJECTS
+#endif
+
int
main(int argc, char **argv)
{
@@ -181,7 +190,9 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, MAX_NUM_WORKERS, &numWorkers))
+ exit(1);
break;
case 'l': /* Dump the TOC summary */
@@ -344,22 +355,6 @@ main(int argc, char **argv)
exit_nicely(1);
}
- if (numWorkers <= 0)
- {
- pg_log_error("invalid number of parallel jobs");
- exit(1);
- }
-
- /* See comments in pg_dump.c */
-#ifdef WIN32
- if (numWorkers > MAXIMUM_WAIT_OBJECTS)
- {
- pg_log_error("maximum number of parallel jobs is %d",
- MAXIMUM_WAIT_OBJECTS);
- exit(1);
- }
-#endif
-
/* Can't do single-txn mode with multiple connections */
if (opts->single_txn && numWorkers > 1)
{
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 64bbda5650..a8cd23f7fa 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -9,12 +9,15 @@
#include "postgres_fe.h"
+#include <limits.h>
#include <time.h>
#ifdef WIN32
#include <io.h>
#endif
+#include "common/logging.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "pg_upgrade.h"
#include "utils/pidfile.h"
@@ -104,6 +107,8 @@ parseCommandLine(int argc, char *argv[])
while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
+ int p;
+
switch (option)
{
case 'b':
@@ -127,7 +132,10 @@ parseCommandLine(int argc, char *argv[])
break;
case 'j':
- user_opts.jobs = atoi(optarg);
+ if (option_parse_int(PG_LOG_OMIT, optarg, "-j/--jobs",
+ 0, INT_MAX, &user_opts.jobs))
+ pg_fatal("-j/--jobs must be an integer in range %d..%d: \"%s\"\n",
+ 0, INT_MAX, optarg);
break;
case 'k':
@@ -166,13 +174,19 @@ parseCommandLine(int argc, char *argv[])
* supported on all old/new versions (added in PG 9.2).
*/
case 'p':
- if ((old_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid old port number\n");
+ if (option_parse_int(PG_LOG_OMIT, optarg, "-p/--old-port",
+ 1, 65535, &p))
+ pg_fatal("old port number must be an integer in range %d..%d: \"%s\"\n",
+ 1, 65535, optarg);
+ old_cluster.port = p;
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
- pg_fatal("invalid new port number\n");
+ if (option_parse_int(PG_LOG_OMIT, optarg, "-P/--new-port",
+ 1, 65535, &p))
+ pg_fatal("new port number must be an integer in range %d..%d: \"%s\"\n",
+ 1, 65535, optarg);
+ new_cluster.port = p;
break;
case 'r':
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 364b5a2e47..faa0969d70 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -63,6 +63,7 @@
#include "common/username.h"
#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -871,7 +872,12 @@ invalid_syntax:
return false;
}
-/* convert string to double, detecting overflows/underflows */
+/*
+ * convert string to double, detecting overflows/underflows
+ *
+ * This is similar to option_parse_double but leave this function for
+ * performance reasons.
+ */
bool
strtodouble(const char *str, bool errorOK, double *dv)
{
@@ -5887,12 +5893,9 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
- {
- pg_log_fatal("invalid number of clients: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-c/--client",
+ 1, INT_MAX, &nclients))
exit(1);
- }
#ifdef HAVE_GETRLIMIT
#ifdef RLIMIT_NOFILE /* most platforms use RLIMIT_NOFILE */
if (getrlimit(RLIMIT_NOFILE, &rlim) == -1)
@@ -5914,12 +5917,10 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
- {
- pg_log_fatal("invalid number of threads: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-j/--jobs",
+ 0, INT_MAX, &nthreads))
exit(1);
- }
+
#ifndef ENABLE_THREAD_SAFETY
if (nthreads != 1)
{
@@ -5938,30 +5939,21 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
- {
- pg_log_fatal("invalid scaling factor: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-s/--scale",
+ 1, INT_MAX, &scale))
exit(1);
- }
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
- {
- pg_log_fatal("invalid number of transactions: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-t/--transactions",
+ 1, INT_MAX, &nxacts))
exit(1);
- }
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
- {
- pg_log_fatal("invalid duration: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-T/--time",
+ 1, INT_MAX, &duration))
exit(1);
- }
break;
case 'U':
username = pg_strdup(optarg);
@@ -6019,12 +6011,9 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
- {
- pg_log_fatal("invalid fillfactor: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-F/--fillfactor",
+ 10, 100, &fillfactor))
exit(1);
- }
break;
case 'M':
benchmarking_option_set = true;
@@ -6039,38 +6028,61 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
- {
- pg_log_fatal("invalid thread progress delay: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "-P/--progress",
+ 1, INT_MAX, &progress))
exit(1);
- }
break;
case 'R':
{
- /* get a double from the beginning of option value */
- double throttle_value = atof(optarg);
+ double throttle_value;
- benchmarking_option_set = true;
+ if (option_parse_double(PG_LOG_FATAL, optarg, "-R/--rate",
+ &throttle_value))
+ exit(1);
- if (throttle_value <= 0.0)
+ /*
+ * This value will be used for yielding int64 number
+ * through some arithmetic. Quite arbitrary but people
+ * never want such small numbers for this parameter.
+ */
+ if (throttle_value <= 1e-5)
{
- pg_log_fatal("invalid rate limit: \"%s\"", optarg);
+ pg_log_fatal("-R/--rate must be greater than 1e-5: \"%s\"", optarg);
exit(1);
}
+
/* Invert rate limit into per-transaction delay in usec */
throttle_delay = 1000000.0 / throttle_value;
}
break;
case 'L':
{
- double limit_ms = atof(optarg);
+ double limit_ms;
+ if (option_parse_double(PG_LOG_FATAL, optarg,
+ "-L/--latency-limit", &limit_ms))
+ exit(1);
+
+ /* limit limit_ms so that latency_limit fits in int64 */
if (limit_ms <= 0.0)
{
- pg_log_fatal("invalid latency limit: \"%s\"", optarg);
+ pg_log_fatal("-L/--latency-limit must be greater than zero: \"%s\"",
+ optarg);
exit(1);
}
+ /*
+ * limit_ms * 1000 must fit int64. We could calculate the
+ * precise limit but also we don't need to accept such a
+ * large number here. Thus use a quite arbitrary seconds
+ * for the limit.
+ */
+ if (limit_ms > 3600)
+ {
+ pg_log_fatal("-L/--latency-limit must be less than 3600 seconds: \"%s\"",
+ optarg);
+ exit(1);
+ }
+
benchmarking_option_set = true;
latency_limit = (int64) (limit_ms * 1000);
}
@@ -6089,21 +6101,21 @@ main(int argc, char **argv)
break;
case 4: /* sampling-rate */
benchmarking_option_set = true;
- sample_rate = atof(optarg);
+ if (option_parse_double(PG_LOG_FATAL, optarg, "--samplig-rate",
+ &sample_rate))
+ exit(1);
if (sample_rate <= 0.0 || sample_rate > 1.0)
{
- pg_log_fatal("invalid sampling rate: \"%s\"", optarg);
+ pg_log_fatal("sampling rate must be greater than zero and less than or equal to 1.0: \"%s\"",
optarg);
exit(1);
}
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
- {
- pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg,
+ "--aggregate-interval",
+ 1, INT_MAX, &agg_interval))
exit(1);
- }
break;
case 6: /* progress-timestamp */
progress_timestamp = true;
@@ -6135,12 +6147,9 @@ main(int argc, char **argv)
break;
case 11: /* partitions */
initialization_option_set = true;
- partitions = atoi(optarg);
- if (partitions < 0)
- {
- pg_log_fatal("invalid number of partitions: \"%s\"", optarg);
+ if (option_parse_int(PG_LOG_FATAL, optarg, "--partitions",
+ 0, INT_MAX, &partitions))
exit(1);
- }
break;
case 12: /* partition-method */
initialization_option_set = true;
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..312a797e78 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -11,6 +11,7 @@
#include <time.h>
#include <pwd.h>
#include <utime.h>
+#include <limits.h>
#ifndef WIN32
#include <sys/stat.h> /* for stat() */
#include <sys/time.h> /* for setitimer() */
@@ -34,6 +35,7 @@
#include "describe.h"
#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "help.h"
#include "input.h"
@@ -1040,11 +1042,19 @@ exec_command_edit(PsqlScanState scan_state, bool active_branch,
}
if (ln)
{
- lineno = atoi(ln);
- if (lineno < 1)
+ switch (option_parse_int(PG_LOG_OMIT, ln, "line number",
+ 1, INT_MAX, &lineno))
{
- pg_log_error("invalid line number: %s", ln);
- status = PSQL_CMD_ERROR;
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ case OPTPARSE_TOOSMALL:
+ case OPTPARSE_TOOLARGE:
+ pg_log_error("line number must be an integer greater than zero: %s", ln);
+ break;
+ case OPTPARSE_OUTOFRANGE:
+ pg_log_error("line number out of range: %s", ln);
+ break;
}
}
if (status != PSQL_CMD_ERROR)
@@ -4284,7 +4294,16 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "border") == 0)
{
if (value)
- popt->topt.border = atoi(value);
+ {
+ int v;
+
+ if (option_parse_int(PG_LOG_OMIT, value, "border", 0, 65535, &v))
+ {
+ pg_log_error("\\pset: border must be an integer in range 0..65535: \"%s\"", value);
+ return false;
+ }
+ popt->topt.border = v;
+ }
}
/* set expanded/vertical mode */
@@ -4440,7 +4459,22 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "pager_min_lines") == 0)
{
if (value)
- popt->topt.pager_min_lines = atoi(value);
+ {
+ switch (option_parse_int(PG_LOG_OMIT, value, "page_min_lines",
+ 0, INT_MAX, &popt->topt.pager_min_lines))
+ {
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ case OPTPARSE_TOOSMALL:
+ case OPTPARSE_TOOLARGE:
+ pg_log_error("\\pset: pager_min_lines must be a non-negative integer: \"%s\"", value);
+ return false;
+ case OPTPARSE_OUTOFRANGE:
+ pg_log_error("\\pset: pager_min_lines out of range: \"%s\"", value);
+ return false;
+ }
+ }
}
/* disable "(x rows)" footer */
@@ -4456,7 +4490,22 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
else if (strcmp(param, "columns") == 0)
{
if (value)
- popt->topt.columns = atoi(value);
+ {
+ switch (option_parse_int(PG_LOG_OMIT, value, "columns",
+ 0, INT_MAX, &popt->topt.columns))
+ {
+ case OPTPARSE_SUCCESS:
+ break;
+ case OPTPARSE_MALFORMED:
+ case OPTPARSE_TOOSMALL:
+ case OPTPARSE_TOOLARGE:
+ pg_log_error("\\pset: column must be a non-negative integer: \"%s\"", value);
+ return false;
+ case OPTPARSE_OUTOFRANGE:
+ pg_log_error("\\pset: column out of range: \"%s\"", value);
+ return false;
+ }
+ }
}
else
{
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index fc0681538a..f510a63157 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -9,6 +9,8 @@
*-------------------------------------------------------------------------
*/
+#include <limits.h>
+
#include "postgres_fe.h"
#include "catalog/pg_class_d.h"
@@ -151,12 +153,9 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, INT_MAX, &concurrentCons))
exit(1);
- }
break;
case 'v':
verbose = true;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index a85919c5c1..21966ed7e2 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -10,6 +10,8 @@
*-------------------------------------------------------------------------
*/
+#include <limits.h>
+
#include "postgres_fe.h"
#include "catalog/pg_class_d.h"
@@ -17,6 +19,7 @@
#include "common.h"
#include "common/connect.h"
#include "common/logging.h"
+#include "common/string.h"
#include "fe_utils/cancel.h"
#include "fe_utils/option_utils.h"
#include "fe_utils/parallel_slot.h"
@@ -192,20 +195,14 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
- {
- pg_log_error("number of parallel jobs must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-j/--jobs",
+ 1, INT_MAX, &concurrentCons))
exit(1);
- }
break;
case 'P':
- vacopts.parallel_workers = atoi(optarg);
- if (vacopts.parallel_workers < 0)
- {
- pg_log_error("parallel workers for vacuum must be greater than or equal to zero");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "-P/--parallel",
+ 0, INT_MAX, &vacopts.parallel_workers))
exit(1);
- }
break;
case 2:
maintenance_db = pg_strdup(optarg);
@@ -220,20 +217,14 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
- {
- pg_log_error("minimum transaction ID age must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "--min-xid-age",
+ 1, INT_MAX, &vacopts.min_xid_age))
exit(1);
- }
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
- {
- pg_log_error("minimum multixact ID age must be at least 1");
+ if (option_parse_int(PG_LOG_ERROR, optarg, "--min-mxid-age",
+ 1, INT_MAX, &vacopts.min_mxid_age))
exit(1);
- }
break;
case 8:
vacopts.no_index_cleanup = true;
diff --git a/src/fe_utils/option_utils.c b/src/fe_utils/option_utils.c
index e19a495dba..4f191d321a 100644
--- a/src/fe_utils/option_utils.c
+++ b/src/fe_utils/option_utils.c
@@ -10,8 +10,12 @@
*-------------------------------------------------------------------------
*/
+#include <limits.h>
+
#include "postgres_fe.h"
+#include "common/string.h"
+#include "common/logging.h"
#include "fe_utils/option_utils.h"
/*
@@ -36,3 +40,151 @@ handle_help_version_opts(int argc, char *argv[],
}
}
}
+
+/*
+ * parser for int/long values in decimal for command line options
+ *
+ * Returns OPTPARSE_SUCCESS (0) when successfully parsed.
+ *
+ * Otherwise returns non-zero status numbers indicating error details after
+ * printing a generic error message with the given errorlevel, which may be
+ * omitted when the errorlevel is lower than the common/logging.c's
+ * setting. Giving PG_LOG_OMIT as loglevel disables generic errors.
+ *
+ * If result is not NULL, the result is stored there only when successfully
+ * parsed.
+ */
+optparse_result
+option_parse_int(int loglevel, const char *optarg, const char *optname,
+ int min_range, int max_range, int *result)
+{
+ char *endptr;
+ int parsed;
+
+ errno = 0;
+ parsed = strtoint(optarg, &endptr, 10);
+
+ if (*endptr)
+ {
+ pg_log_level(loglevel, "%s must be an integer: \"%s\"",
+ optname, optarg);
+ return OPTPARSE_MALFORMED;
+ }
+ else if (errno == ERANGE || parsed < min_range || parsed > max_range)
+ {
+ if (max_range == INT_MAX)
+ {
+ if (errno == ERANGE)
+ pg_log_level(loglevel, "%s out of range: %s",
+ optname, optarg);
+ else
+ pg_log_level(loglevel, "%s must be at least %d: %s",
+ optname, min_range, optarg);
+ }
+ else
+ pg_log_level(loglevel, "%s must be in range %d..%d: %s",
+ optname, min_range, max_range, optarg);
+
+ if (errno == ERANGE)
+ return OPTPARSE_OUTOFRANGE;
+ if (parsed < min_range)
+ return OPTPARSE_TOOSMALL;
+
+ Assert (parsed > max_range);
+ return OPTPARSE_TOOLARGE;
+ }
+
+ if (result)
+ *result = parsed;
+
+ return OPTPARSE_SUCCESS;
+}
+
+
+optparse_result
+option_parse_long(int loglevel, const char *optarg, const char *optname,
+ long min_range, long max_range, long *result)
+{
+ char *endptr;
+ long parsed;
+
+ errno = 0;
+ parsed = strtol(optarg, &endptr, 10);
+
+ if (*endptr)
+ {
+ pg_log_level(loglevel, "%s must be an integer: \"%s\"",
+ optname, optarg);
+ return OPTPARSE_MALFORMED;
+ }
+ else if (errno == ERANGE || parsed < min_range || parsed > max_range)
+ {
+ if (max_range == LONG_MAX)
+ {
+ if (errno == ERANGE)
+ pg_log_level(loglevel, "%s out of range: %s",
+ optname, optarg);
+ else
+ pg_log_level(loglevel, "%s must be at least %ld: %s",
+ optname, min_range, optarg);
+ }
+ else
+ pg_log_level(loglevel, "%s must be in range %ld..%ld: %s",
+ optname, min_range, max_range, optarg);
+
+ if (errno == ERANGE)
+ return OPTPARSE_OUTOFRANGE;
+ if (parsed < min_range)
+ return OPTPARSE_TOOSMALL;
+
+ Assert (parsed > max_range);
+ return OPTPARSE_TOOLARGE;
+ }
+
+ if (result)
+ *result = parsed;
+
+ return OPTPARSE_SUCCESS;
+}
+
+
+/*
+ * parser for double value in decimal for command line options
+ *
+ * Returns OPTPARSE_SUCCESS (0) when successfully parsed.
+ *
+ * Otherwise returns non-zero status numbers indicating error details after
+ * printing a fixed-form error message with the given errorlevel, which may be
+ * omitted when the errorlevel is lower than the common/logging.c's
+ * setting. Giving PG_LOG_OMIT as loglevel disables this error output.
+ *
+ * If result is not NULL, the result is stored there only when successfully
+ * parsed.
+ */
+optparse_result
+option_parse_double(int loglevel, const char *optarg, const char *optname,
+ double *result)
+{
+ char *endptr;
+ double parsed;
+
+ errno = 0;
+ parsed = strtod(optarg, &endptr);
+
+ if (*endptr)
+ {
+ pg_log_level(loglevel, "%s must be a numeric: \"%s\"", optname, optarg);
+ return OPTPARSE_MALFORMED;
+ }
+ else if (errno == ERANGE)
+ {
+ pg_log_level(loglevel, "%s out of range: %s", optname, optarg);
+
+ return OPTPARSE_OUTOFRANGE;
+ }
+
+ if (result)
+ *result = parsed;
+
+ return OPTPARSE_SUCCESS;
+}
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index a71cf84249..c2e9d1e46f 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -15,6 +15,11 @@
*/
enum pg_log_level
{
+ /*
+ * Don't show this message
+ */
+ PG_LOG_OMIT = -1,
+
/*
* Not initialized yet
*/
@@ -93,4 +98,8 @@ void pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_
if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
} while(0)
+#define pg_log_level(loglevel, ...) do { \
+ if (unlikely(__pg_log_level <= loglevel)) pg_log_generic(loglevel, __VA_ARGS__); \
+ } while(0)
+
#endif /* COMMON_LOGGING_H */
diff --git a/src/include/fe_utils/option_utils.h b/src/include/fe_utils/option_utils.h
index d653cb94e3..bfba7a73ed 100644
--- a/src/include/fe_utils/option_utils.h
+++ b/src/include/fe_utils/option_utils.h
@@ -14,10 +14,33 @@
#include "postgres_fe.h"
+typedef enum optparse_result
+{
+ OPTPARSE_SUCCESS = 0,
+ OPTPARSE_MALFORMED,
+ OPTPARSE_TOOSMALL,
+ OPTPARSE_TOOLARGE,
+ OPTPARSE_OUTOFRANGE
+} optparse_result;
+
typedef void (*help_handler) (const char *progname);
extern void handle_help_version_opts(int argc, char *argv[],
const char *fixed_progname,
help_handler hlp);
+extern optparse_result option_parse_int(int loglevel,
+ const char *optarg, const char *optname,
+ int min_range, int max_range,
+ int *result);
+
+extern optparse_result option_parse_long(int loglevel,
+ const char *optarg,
+ const char *optname,
+ long min_range, long max_range,
+ long *result);
+
+extern optparse_result option_parse_double(int loglevel, const char *optarg,
+ const char *optname, double *result);
+
#endif /* OPTION_UTILS_H */
--
2.27.0
From 972567547f15e0f47512d08d97ce01aff4bc05a7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 17:10:19 +0900
Subject: [PATCH v3 2/3] Make complain for invalid numeirc values in
environemnt variables
Postgresql commands that read environment variables for integer values
are tolerant of trailing garbages like '5432xyz'. Ignore such values
and make warning on it.
---
src/bin/pg_ctl/pg_ctl.c | 6 ++++--
src/bin/pg_upgrade/option.c | 26 ++++++++++++++++++++++++--
src/bin/psql/startup.c | 11 ++++++++++-
3 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index f7ccdcdc96..b3c3e66bda 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2320,8 +2320,10 @@ main(int argc, char **argv)
#endif
env_wait = getenv("PGCTLTIMEOUT");
- if (env_wait != NULL)
- wait_seconds = atoi(env_wait);
+ if (env_wait != NULL &&
+ option_parse_int(PG_LOG_OMIT, env_wait, "PGCTLTIMEOUT",
+ 0, 65535, &wait_seconds))
+ write_stderr(_("%s: ignored invalid setting of environment variable PGCTLTIMEOUT: %s\n"), progname,
env_wait);
/*
* 'Action' can be before or after args so loop over both. Some
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index a8cd23f7fa..e506c3e2af 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -74,8 +74,30 @@ parseCommandLine(int argc, char *argv[])
os_info.progname = get_progname(argv[0]);
/* Process libpq env. variables; load values here for usage() output */
- old_cluster.port = getenv("PGPORTOLD") ? atoi(getenv("PGPORTOLD")) : DEF_PGUPORT;
- new_cluster.port = getenv("PGPORTNEW") ? atoi(getenv("PGPORTNEW")) : DEF_PGUPORT;
+ old_cluster.port = new_cluster.port = DEF_PGUPORT;
+ if (getenv("PGPORTOLD"))
+ {
+ int p;
+
+ if (option_parse_int(PG_LOG_OMIT, getenv("PGPORTOLD"), "PGPORTOLD",
+ 0, 65535, &p))
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTOLD: %s\n",
+ os_info.progname, getenv("PGPORTOLD"));
+ old_cluster.port = p;
+ }
+
+ if (getenv("PGPORTNEW"))
+ {
+ int p;
+
+ if (option_parse_int(PG_LOG_OMIT, getenv("PGPORTNEW"), "PGPORTNEW",
+ 0, 65535, &p))
+ pg_log(PG_WARNING,
+ "%s: ignored invalid setting of environment variable PGPORTNEW: %s\n",
+ os_info.progname, getenv("PGPORTNEW"));
+ new_cluster.port = p;
+ }
os_user_effective_id = get_user_info(&os_info.user);
/* we override just the database user name; we got the OS id above */
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 2931530f33..819d5493c9 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -13,12 +13,14 @@
#include <io.h>
#include <win32.h>
#endif /* WIN32 */
+#include <limits.h>
#include "command.h"
#include "common.h"
#include "common/logging.h"
#include "common/string.h"
#include "describe.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/print.h"
#include "getopt_long.h"
#include "help.h"
@@ -181,7 +183,14 @@ main(int argc, char *argv[])
refresh_utf8format(&(pset.popt.topt));
/* We must get COLUMNS here before readline() sets it */
- pset.popt.topt.env_columns = getenv("COLUMNS") ? atoi(getenv("COLUMNS")) : 0;
+ pset.popt.topt.env_columns = 0;
+ if (getenv("COLUMNS"))
+ {
+ if (option_parse_int(PG_LOG_OMIT, getenv("COLUMNS"), "COLUMNS",
+ 0, INT_MAX, &pset.popt.topt.env_columns))
+ pg_log_warning("ignored invalid setting of environemt variable COLUMNS: %s",
+ getenv("COLUMNS"));
+ }
pset.notty = (!isatty(fileno(stdin)) || !isatty(fileno(stdout)));
--
2.27.0
From ae7276bd8df4e9d9f1d37156cfa29f1130698403 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 13 Jul 2021 19:35:12 +0900
Subject: [PATCH v3 3/3] Doc change for pg_ctl
The previous patch change the bahavior of pg_ctl about
--timeout. Change the doc following the behavior change.
---
doc/src/sgml/ref/pg_ctl-ref.sgml | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml
index 3946fa52ea..90e0fabc63 100644
--- a/doc/src/sgml/ref/pg_ctl-ref.sgml
+++ b/doc/src/sgml/ref/pg_ctl-ref.sgml
@@ -389,9 +389,11 @@ PostgreSQL documentation
<listitem>
<para>
Specifies the maximum number of seconds to wait when waiting for an
- operation to complete (see option <option>-w</option>). Defaults to
- the value of the <envar>PGCTLTIMEOUT</envar> environment variable or, if
- not set, to 60 seconds.
+ operation to complete (see option <option>-w</option>). If it is set
+ to zero, <command>pg_ctl</command> behaves the same way
+ with <option>-W</option>. Defaults to the value of
+ the <envar>PGCTLTIMEOUT</envar> environment variable or, if not set,
+ to 60 seconds.
</para>
</listitem>
</varlistentry>
--
2.27.0
On Wed, 21 Jul 2021 at 23:50, Michael Paquier <michael@paquier.xyz> wrote: > Hacking on that, I am finishing with the attached. It is less > ambitious, still very useful as it removes a dozen of custom error > messages in favor of the two ones introduced in option_utils.c. On > top of that this reduces a bit the code: > 15 files changed, 156 insertions(+), 169 deletions(-) > > What do you think? This is just a driveby review, but I think that it's good to see no increase in the number of lines of code to make these improvements. It's also good to see the added consistency introduced by this patch. I didn't test the patch, so this is just from reading through. I wondered about the TAP tests here: command_fails_like( [ 'pg_dump', '-j', '-1' ], qr/\Qpg_dump: error: -j\/--jobs must be in range 0..2147483647\E/, 'pg_dump: invalid number of parallel jobs'); command_fails_like( [ 'pg_restore', '-j', '-1', '-f -' ], qr/\Qpg_restore: error: -j\/--jobs must be in range 0..2147483647\E/, 'pg_restore: invalid number of parallel jobs'); I see both of these are limited to 64 on windows. Won't those fail on Windows? I also wondered if it would be worth doing #define MAX_JOBS somewhere away from the option parsing code. This part is pretty ugly: /* * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS * (= 64 usually) parallel jobs because that's the maximum * limit for the WaitForMultipleObjects() call. */ if (!option_parse_int(optarg, "-j/--jobs", 0, #ifdef WIN32 MAXIMUM_WAIT_OBJECTS, #else INT_MAX, #endif &numWorkers)) exit(1); break; David
On Thu, Jul 22, 2021 at 12:32:39AM +1200, David Rowley wrote:
> I see both of these are limited to 64 on windows. Won't those fail on Windows?
Yes, thanks, they would. I would just cut the range numbers from the
expected output here. This does not matter in terms of coverage
either.
x> I also wondered if it would be worth doing #define MAX_JOBS somewhere
> away from the option parsing code. This part is pretty ugly:
Agreed as well. pg_dump and pg_restore have their own idea of
parallelism in parallel.{c.h}. What about putting MAX_JOBS in
parallel.h then?
--
Michael
Attachment
On Thu, 22 Jul 2021 at 00:44, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 22, 2021 at 12:32:39AM +1200, David Rowley wrote:
> > I see both of these are limited to 64 on windows. Won't those fail on Windows?
>
> Yes, thanks, they would. I would just cut the range numbers from the
> expected output here. This does not matter in terms of coverage
> either.
Sounds good.
> x> I also wondered if it would be worth doing #define MAX_JOBS somewhere
> > away from the option parsing code. This part is pretty ugly:
>
> Agreed as well. pg_dump and pg_restore have their own idea of
> parallelism in parallel.{c.h}. What about putting MAX_JOBS in
> parallel.h then?
parallel.h looks ok to me.
David
On Thu, Jul 22, 2021 at 01:19:41AM +1200, David Rowley wrote:
> On Thu, 22 Jul 2021 at 00:44, Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Thu, Jul 22, 2021 at 12:32:39AM +1200, David Rowley wrote:
>> > I see both of these are limited to 64 on windows. Won't those fail on Windows?
>>
>> Yes, thanks, they would. I would just cut the range numbers from the
>> expected output here. This does not matter in terms of coverage
>> either.
>
> Sounds good.
>
>> x> I also wondered if it would be worth doing #define MAX_JOBS somewhere
>> > away from the option parsing code. This part is pretty ugly:
>>
>> Agreed as well. pg_dump and pg_restore have their own idea of
>> parallelism in parallel.{c.h}. What about putting MAX_JOBS in
>> parallel.h then?
>
> parallel.h looks ok to me.
Okay, done those parts as per the attached. While on it, I noticed an
extra one for pg_dump --rows-per-insert. I am counting 25 translated
strings cut in total.
Any objections to this first step?
--
Michael
Attachment
On Thu, Jul 22, 2021 at 02:32:35PM +0900, Michael Paquier wrote: > Okay, done those parts as per the attached. While on it, I noticed an > extra one for pg_dump --rows-per-insert. I am counting 25 translated > strings cut in total. > > Any objections to this first step? I have looked at that over the last couple of days, and applied it after some small fixes, including an indentation. The int64 and float parts are extra types we could handle, but I have not looked yet at how much benefits we'd get in those cases. -- Michael
Attachment
On Sat, Jul 24, 2021 at 07:41:12PM +0900, Michael Paquier wrote: > I have looked at that over the last couple of days, and applied it > after some small fixes, including an indentation. One thing that we forgot here is the handling of trailing whitespaces. Attached is small patch to adjust that, with one positive and one negative tests. > The int64 and float > parts are extra types we could handle, but I have not looked yet at > how much benefits we'd get in those cases. I have looked at these two but there is really less benefits, so for now I am not planning to do more in this area. For float options, pg_basebackup --max-rate could be one target on top of the three set of options in pgbench, but it needs to handle units :( -- Michael
Attachment
At Mon, 26 Jul 2021 15:01:35 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Sat, Jul 24, 2021 at 07:41:12PM +0900, Michael Paquier wrote: > > I have looked at that over the last couple of days, and applied it > > after some small fixes, including an indentation. > > One thing that we forgot here is the handling of trailing > whitespaces. Attached is small patch to adjust that, with one > positive and one negative tests. > > > The int64 and float > > parts are extra types we could handle, but I have not looked yet at > > how much benefits we'd get in those cases. > > I have looked at these two but there is really less benefits, so for > now I am not planning to do more in this area. For float options, > pg_basebackup --max-rate could be one target on top of the three set > of options in pgbench, but it needs to handle units :( Thanks for revising and committing! I'm fine with all of the recent discussions on the committed part. Though I don't think it works for "live" command line options, making the omitting policy symmetric looks good. I see the same done in several similar use of strto[il]. The change in 020_pg_receivewal.pl results in a chain of four successive failures, which is fine. But the last failure (#23) happens for a bit dubious reason. > Use of uninitialized value in pattern match (m//) at t/020_pg_receivewal.pl line 114. > not ok 23 - one partial WAL segment is now completed It might not be worth amending, but we don't need to use m/// there and eq works fine. 020_pg_receivewal.pl: 114 - is($zlib_wals[0] =~ m/$partial_wals[0]/, + is($zlib_wals[0] eq $partial_wals[0], regards. -- Kyotaro Horiguchi NTT Open Source Software Center