Re: Change atoi to strtol in same place - Mailing list pgsql-hackers
| From | Joe Nelson |
|---|---|
| Subject | Re: Change atoi to strtol in same place |
| Date | |
| Msg-id | 20190724040237.GB64205@begriffs.com Whole thread Raw |
| In response to | Re: Change atoi to strtol in same place (Tomas Vondra <tomas.vondra@2ndquadrant.com>) |
| Responses |
Re: Change atoi to strtol in same place
(David Rowley <david.rowley@2ndquadrant.com>)
|
| List | pgsql-hackers |
> Surafel Temesgen wrote: > > we use atoi for user argument processing in same place which return > > zero for both invalid input and input value zero. [...] in same > > place where we accept zero as valued input it case a problem by > > preceding for invalid input as input value zero. strtol which can > > handle invalid input Not only that, but atoi causes Undefined Behavior on erroneous input. The C99 standard says this: 7.20.1 Numeric conversion functions The functions atof, atoi, atol, and atoll need not affect the value of the integer expression errno on an error. If the value of the result cannot be represented, the behavior is undefined. Tomas Vondra wrote: > This seems to have bit-rotted (due to minor changes to pg_basebackup). > Can you fix that and post an updated version? I adjusted the patch to apply cleanly on a0555ddab9. > In general, I think it's a good idea to fix those places, but I wonder > if we need to change the error messages too. I'll leave that decision for the community to debate. I did, however, remove the newlines for the new error messages being passed to pg_log_error(). As discussed in message [0], the logging functions in common/logging.c now contain an assertion that messages do not end in newline: Assert(fmt[strlen(fmt) - 1] != '\n'); (in pg_log_error via pg_log_generic via pg_log_generic_v) I also added limits.h to some places it was missing, so the patch would build. 0: https://postgr.es/m/6a609b43-4f57-7348-6480-bd022f924310@2ndquadrant.com diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c index 23f706b21d..2bcacbfbb5 100644 --- a/contrib/pg_standby/pg_standby.c +++ b/contrib/pg_standby/pg_standby.c @@ -27,6 +27,7 @@ #include <dirent.h> #include <sys/stat.h> #include <fcntl.h> +#include <limits.h> #include <signal.h> #include <sys/time.h> @@ -638,6 +639,14 @@ sigquit_handler(int sig) int main(int argc, char **argv) { + char *keepfilesendptr; + char *maxretriesendptr; + char *sleeptimeendptr; + char *maxwaittimeendptr; + long numkeepfiles; + long nummaxretries; + long numsleeptime; + long nummaxwaittime; int c; progname = get_progname(argv[0]); @@ -688,12 +697,17 @@ main(int argc, char **argv) debug = true; break; case 'k': /* keepfiles */ - keepfiles = atoi(optarg); - if (keepfiles < 0) + errno = 0; + numkeepfiles = strtol(optarg, &keepfilesendptr, 10); + + if (keepfilesendptr == optarg || *keepfilesendptr != '\0' || + numkeepfiles < 0 || numkeepfiles > INT_MAX || + errno == ERANGE) { - fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname); + fprintf(stderr, "%s: -k keepfiles must be in range %d..%d\n", progname, 0, INT_MAX); exit(2); } + keepfiles = (int) numkeepfiles; break; case 'l': /* Use link */ @@ -707,31 +721,46 @@ main(int argc, char **argv) #endif break; case 'r': /* Retries */ - maxretries = atoi(optarg); - if (maxretries < 0) + errno = 0; + nummaxretries = strtol(optarg, &maxretriesendptr, 10); + + if (maxretriesendptr == optarg || *maxretriesendptr != '\0' || + nummaxretries < 0 || nummaxretries > INT_MAX || + errno == ERANGE) { - fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname); + fprintf(stderr, "%s: -r maxretries must be in range %d..%d\n", progname, 0, INT_MAX); exit(2); } + maxretries = (int) nummaxretries; break; case 's': /* Sleep time */ - sleeptime = atoi(optarg); - if (sleeptime <= 0 || sleeptime > 60) + errno = 0; + numsleeptime = strtol(optarg, &sleeptimeendptr, 10); + + if (sleeptimeendptr == optarg || *sleeptimeendptr != '\0' || + numsleeptime <= 0 || numsleeptime > 60 || + errno == ERANGE) { - fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname); + fprintf(stderr, "%s: -s sleeptime must be in range %d..%d\n", progname, 1, 59); exit(2); } + sleeptime = (int) numsleeptime; break; case 't': /* Trigger file */ triggerPath = pg_strdup(optarg); break; case 'w': /* Max wait time */ - maxwaittime = atoi(optarg); - if (maxwaittime < 0) + errno = 0; + nummaxwaittime = strtol(optarg, &maxwaittimeendptr, 10); + + if (maxwaittimeendptr == optarg || *maxwaittimeendptr != '\0' || + nummaxwaittime < 0 || nummaxwaittime > INT_MAX || + errno == ERANGE) { - fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname); + fprintf(stderr, "%s: -w maxwaittime must be in range %d..%d\n", progname, 0, INT_MAX); exit(2); } + maxwaittime = (int) nummaxwaittime; break; default: fprintf(stderr, "Try \"%s --help\" for more information.\n", progname); diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 77a7c148ba..6ed523fdd6 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> @@ -2197,6 +2198,10 @@ main(int argc, char **argv) {"no-verify-checksums", no_argument, NULL, 3}, {NULL, 0, NULL, 0} }; + char *compressEndptr; + char *timeoutEndptr; + long compressNumber; + long timeoutNumber; int c; int option_index; @@ -2309,12 +2314,18 @@ main(int argc, char **argv) #endif break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) + errno = 0; + compressNumber = strtol(optarg, &compressEndptr, 10); + + if (compressEndptr == optarg || *compressEndptr != '\0' || + compressNumber < 0 || compressNumber > 9 || + errno == ERANGE) { - pg_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("compression level must be in range %d..%d \"%s\"", + 0, 9, optarg); exit(1); } + compresslevel = (int) compressNumber; break; case 'c': if (pg_strcasecmp(optarg, "fast") == 0) @@ -2347,12 +2358,18 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + timeoutNumber = strtol(optarg, &timeoutEndptr, 10); + + if (timeoutEndptr == optarg || *timeoutEndptr != '\0' || + timeoutNumber < 0 || timeoutNumber > INT_MAX || + errno == ERANGE) { - pg_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("status interval must be in range %d..%d \"%s\"", + 0, INT_MAX, optarg); exit(1); } + standby_message_timeout = (int) timeoutNumber * 1000; break; case 'v': verbose++; diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index f39c1339d7..94a20f50a2 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> @@ -492,10 +493,15 @@ main(int argc, char **argv) {"no-sync", no_argument, NULL, 5}, {NULL, 0, NULL, 0} }; - + long compressNumber; + long timeoutNumber; + long portNumber; int c; int option_index; + char *compressEndptr; char *db_name; + char *timeoutEndptr; + char *portEndptr; uint32 hi, lo; @@ -533,9 +539,15 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) + errno = 0; + portNumber = strtol(optarg, &portEndptr, 10); + + if (portEndptr == optarg || *portEndptr != '\0' || + portNumber <= 0 || portNumber > INT_MAX || + errno == ERANGE) { - pg_log_error("invalid port number \"%s\"", optarg); + pg_log_error("port number must be in range %d..%d \"%s\"", + 1, INT_MAX, optarg); exit(1); } dbport = pg_strdup(optarg); @@ -550,12 +562,18 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + timeoutNumber = strtol(optarg, &timeoutEndptr, 10); + + if (timeoutEndptr == optarg || *timeoutEndptr != '\0' || + timeoutNumber < 0 || timeoutNumber > INT_MAX || + errno == ERANGE) { - pg_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("status interval must be in range %d..%d \"%s\"", + 0, INT_MAX, optarg); exit(1); } + standby_message_timeout = (int) timeoutNumber * 1000; break; case 'S': replication_slot = pg_strdup(optarg); @@ -575,12 +593,18 @@ main(int argc, char **argv) verbose++; break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) + errno = 0; + compressNumber = strtol(optarg, &compressEndptr, 10); + + if (compressEndptr == optarg || *compressEndptr != '\0' || + compressNumber < 0 || compressNumber > 9 || + errno == ERANGE) { - pg_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("compression level must be in range %d..%d \"%s\"", + 0, 9, optarg); exit(1); } + compresslevel = (int) compressNumber; break; /* action */ case 1: diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index a10bc8d545..04f2072bb5 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> @@ -2266,6 +2267,8 @@ main(int argc, char **argv) }; char *env_wait; + char *seconds_endptr; + long seconds; int option_index; int c; pgpid_t killproc = 0; @@ -2395,7 +2398,18 @@ main(int argc, char **argv) #endif break; case 't': - wait_seconds = atoi(optarg); + errno = 0; + seconds = strtol(optarg, &seconds_endptr, 10); + + if (seconds_endptr == optarg || *seconds_endptr != '\0' || + seconds <= 0 || seconds > INT_MAX || + errno == ERANGE) + { + write_stderr(_("timeout must be in range %d..%d\n"), + 1, INT_MAX); + exit(1); + } + wait_seconds = (int) seconds; wait_seconds_arg = true; break; case 'U': diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index bbf44a1820..577b6554cc 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -311,14 +311,20 @@ main(int argc, char **argv) DumpableObject *boundaryObjs; int i; int optindex; + char *compressEndptr; + char *digitsEndptr; char *endptr; + char *workersEndptr; RestoreOptions *ropt; Archive *fout; /* the script file */ bool g_verbose = false; const char *dumpencoding = NULL; const char *dumpsnapshot = NULL; char *use_role = NULL; + long compressNumber; + long floatDigits; long rowsPerInsert; + long workersNumber; int numWorkers = 1; trivalue prompt_password = TRI_DEFAULT; int compressLevel = -1; @@ -473,7 +479,18 @@ main(int argc, char **argv) break; case 'j': /* number of dump jobs */ - numWorkers = atoi(optarg); + errno = 0; + workersNumber = strtol(optarg, &workersEndptr, 10); + + if (workersEndptr == optarg || *workersEndptr != '\0' || + workersNumber <= 0 || workersNumber > INT_MAX || + errno == ERANGE) + { + pg_log_error("jobs must be in range %d..%d", + 1, INT_MAX); + exit_nicely(1); + } + numWorkers = (int) workersNumber; break; case 'n': /* include schema(s) */ @@ -536,12 +553,17 @@ main(int argc, char **argv) break; case 'Z': /* Compression Level */ - compressLevel = atoi(optarg); - if (compressLevel < 0 || compressLevel > 9) + errno = 0; + compressNumber = strtol(optarg, &compressEndptr, 10); + + if (compressEndptr == optarg || *compressEndptr != '\0' || + compressNumber < 0 || compressNumber > 9 || + errno == ERANGE) { pg_log_error("compression level must be in range 0..9"); exit_nicely(1); } + compressLevel = (int) compressNumber; break; case 0: @@ -573,13 +595,18 @@ main(int argc, char **argv) break; case 8: - have_extra_float_digits = true; - extra_float_digits = atoi(optarg); - if (extra_float_digits < -15 || extra_float_digits > 3) + errno = 0; + floatDigits = strtol(optarg, &digitsEndptr, 10); + + if (digitsEndptr == optarg || *digitsEndptr != '\0' || + floatDigits < -15 || floatDigits > 3 || + errno == ERANGE) { pg_log_error("extra_float_digits must be in range -15..3"); exit_nicely(1); } + have_extra_float_digits = true; + extra_float_digits = (int) floatDigits; break; case 9: /* inserts */ diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index f9b1ae6809..8f52bd7b3b 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -41,6 +41,7 @@ #include "postgres_fe.h" #include <ctype.h> +#include <limits.h> #ifdef HAVE_TERMIOS_H #include <termios.h> #endif @@ -60,9 +61,11 @@ int main(int argc, char **argv) { RestoreOptions *opts; + long workersNumber; int c; int exit_code; int numWorkers = 1; + char *workersEndptr; Archive *AH; char *inputFileSpec; static int disable_triggers = 0; @@ -185,7 +188,18 @@ main(int argc, char **argv) break; case 'j': /* number of restore jobs */ - numWorkers = atoi(optarg); + errno = 0; + workersNumber = strtol(optarg, &workersEndptr, 10); + + if (workersEndptr == optarg || *workersEndptr != '\0' || + workersNumber <= 0 || workersNumber > INT_MAX || + errno == ERANGE) + { + pg_log_error("jobs must be in range %d..%d", + 1, INT_MAX); + exit_nicely(1); + } + numWorkers = (int) workersNumber; break; case 'l': /* Dump the TOC summary */ diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index d76f27c9e8..0a3ec2c653 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -9,6 +9,7 @@ #include "postgres_fe.h" +#include <limits.h> #include <time.h> #ifdef WIN32 #include <io.h> @@ -59,11 +60,17 @@ parseCommandLine(int argc, char *argv[]) {NULL, 0, NULL, 0} }; + long workersNumber; + long newPortNumber; + long oldNumber; int option; /* Command line option */ int optindex = 0; /* used by getopt_long */ int os_user_effective_id; FILE *fp; char **filename; + char *newPortEndptr; + char *oldPortEndptr; + char *workersEndptr; time_t run_time = time(NULL); user_opts.transfer_mode = TRANSFER_MODE_COPY; @@ -127,7 +134,18 @@ parseCommandLine(int argc, char *argv[]) break; case 'j': - user_opts.jobs = atoi(optarg); + errno = 0; + workersNumber = strtol(optarg, &workersEndptr, 10); + + if (workersEndptr == optarg || *workersEndptr != '\0' || + workersNumber <= 0 || workersNumber > INT_MAX || + errno == ERANGE) + { + pg_fatal("jobs must be in range %d..%d", + 0, INT_MAX); + exit(1); + } + user_opts.jobs = (int) workersNumber; break; case 'k': @@ -166,19 +184,31 @@ 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) + errno = 0; + oldNumber = strtol(optarg, &oldPortEndptr, 10); + + if (oldPortEndptr == optarg || *oldPortEndptr != '\0' || + oldNumber <= 0 || oldNumber > INT_MAX || + errno == ERANGE) { pg_fatal("invalid old port number\n"); exit(1); } + old_cluster.port = (int) oldNumber; break; case 'P': - if ((new_cluster.port = atoi(optarg)) <= 0) + errno = 0; + newPortNumber = strtol(optarg, &newPortEndptr, 10); + + if (newPortEndptr == optarg || *newPortEndptr != '\0' || + newPortNumber <= 0 || newPortNumber > INT_MAX || + errno == ERANGE) { pg_fatal("invalid new port number\n"); exit(1); } + new_cluster.port = (int) newPortNumber; break; case 'r':
pgsql-hackers by date: