Small patch for pg_basebackup argument parsing
Hi
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next commit fest
or is it not needed for such small patches ?
Thanks
Pierre
Attachments:
0001-Check-the-results-of-atoi-in-pg_basebackup.patchtext/x-patch; charset=UTF-8; name=0001-Check-the-results-of-atoi-in-pg_basebackup.patchDownload
From=20c5301085514b8e5acd9ffa5b10ae6521677b4d72 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet <p.psql@pinaraf.info>
Date: Thu, 13 Apr 2017 23:25:51 +0200
Subject: [PATCH] Check the results of atoi in pg_basebackup
Passing invalid strings in integer parameters does not trigger any error
in pg_basebackup right now. This behaviour could be misleading and give
the impression that a setting is considered (-z max, -s 0.5) while it
was reset to 0 instead.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..e69dbd1ed6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
--
2.11.0
0001-Check-the-results-of-atoi-in-pg_basebackup.patchtext/x-patch; charset=UTF-8; name=0001-Check-the-results-of-atoi-in-pg_basebackup.patchDownload
From c5301085514b8e5acd9ffa5b10ae6521677b4d72 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet <p.psql@pinaraf.info>
Date: Thu, 13 Apr 2017 23:25:51 +0200
Subject: [PATCH] Check the results of atoi in pg_basebackup
Passing invalid strings in integer parameters does not trigger any error
in pg_basebackup right now. This behaviour could be misleading and give
the impression that a setting is considered (-z max, -s 0.5) while it
was reset to 0 instead.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..e69dbd1ed6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
--
2.11.0
On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet <p.psql@pinaraf.info> wrote:
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next commit fest
or is it not needed for such small patches ?
A call to atoi is actually equivalent to strtol with the rounding:
(int)strtol(str, (char **)NULL, 10);
So I don't think this is worth caring.
By doing a git grep "atoi(optarg)" you'll see far more places that
handle integer options this way as well...
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next commit
fest or is it not needed for such small patches ?A call to atoi is actually equivalent to strtol with the rounding:
(int)strtol(str, (char **)NULL, 10);
So I don't think this is worth caring.
The problem with atoi is that it simply ignores any invalid input and returns
0 instead.
That's why I did this patch, because I did a typo when calling pg_basebackup
and was not warned for an invalid input.
But it doesn't matter that much, so if you don't think that's interesting, no
problem.
On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet <p.psql@pinaraf.info> wrote:
On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next commit
fest or is it not needed for such small patches ?A call to atoi is actually equivalent to strtol with the rounding:
(int)strtol(str, (char **)NULL, 10);
So I don't think this is worth caring.The problem with atoi is that it simply ignores any invalid input and returns
0 instead.
That's why I did this patch, because I did a typo when calling pg_basebackup
and was not warned for an invalid input.
I agree. I think it would be worth going through and cleaning up
every instance of this in the source tree.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote:
On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user
who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next
commit
fest or is it not needed for such small patches ?A call to atoi is actually equivalent to strtol with the rounding:
(int)strtol(str, (char **)NULL, 10);
So I don't think this is worth caring.The problem with atoi is that it simply ignores any invalid input and
returns 0 instead.
That's why I did this patch, because I did a typo when calling
pg_basebackup and was not warned for an invalid input.I agree. I think it would be worth going through and cleaning up
every instance of this in the source tree.
Well, I don't have much to do during a train travel next week. I'll look into
it and post my results here.
On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote:
On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user
who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next
commit
fest or is it not needed for such small patches ?A call to atoi is actually equivalent to strtol with the rounding:
(int)strtol(str, (char **)NULL, 10);
So I don't think this is worth caring.The problem with atoi is that it simply ignores any invalid input and
returns 0 instead.
That's why I did this patch, because I did a typo when calling
pg_basebackup and was not warned for an invalid input.I agree. I think it would be worth going through and cleaning up
every instance of this in the source tree.
Hi
Following your advice, I went through the source tree and cleaned up most
instances of that pattern.
I have attached the corresponding patch to this mail.
If you think this patch is indeed interesting, what would be the next way to
have it reviewed ?
Thanks
Pierre
Attachments:
0001-Port-most-calls-of-atoi-optarg-to-strcol.patchtext/x-patch; charset=UTF-8; name=0001-Port-most-calls-of-atoi-optarg-to-strcol.patchDownload
From=20da448337bd050518323b45fd5d2929d04b552802 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet <pierre.ducroquet@people-doc.com>
Date: Tue, 18 Apr 2017 09:40:40 +0200
Subject: [PATCH] Port most calls of atoi(optarg) to strcol
atoi does not allow any kind of data check. If the input data is invalid,
the result will be silently truncated to the valid part of input, or just
0 if no digit was found at the beginning of input.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++++++-----
src/bin/pg_basebackup/pg_receivewal.c | 11 ++++++-----
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++++++-----
src/bin/pg_ctl/pg_ctl.c | 8 +++++++-
src/bin/pg_dump/pg_dump.c | 12 +++++++++---
src/bin/pg_dump/pg_restore.c | 8 +++++++-
src/bin/pg_upgrade/option.c | 5 +++--
src/bin/pgbench/pgbench.c | 34 ++++++++++++++++++----------------
8 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..34884fce4f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 1a9fe81be1..f470406678 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -479,6 +479,7 @@ main(int argc, char **argv)
int c;
int option_index;
char *db_name;
+ char *strtol_endptr = NULL;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -513,7 +514,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if ((strtol(optarg, &strtol_endptr, 10) <= 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -531,8 +532,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
@@ -549,8 +550,8 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 6b081bd737..cd69a6fba6 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -706,6 +706,7 @@ main(int argc, char **argv)
uint32 hi,
lo;
char *db_name;
+ char *strtol_endptr;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -735,8 +736,8 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ fsync_interval = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (fsync_interval < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid fsync interval \"%s\"\n"),
progname, optarg);
@@ -757,7 +758,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if (strtol(optarg, &strtol_endptr, 10) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -819,8 +820,8 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (standby_message_timeout < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index c63819b88b..65191a5319 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2177,6 +2177,7 @@ main(int argc, char **argv)
};
char *env_wait;
+ char *strtol_endptr;
int option_index;
int c;
pgpid_t killproc = 0;
@@ -2307,7 +2308,12 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ wait_seconds = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg)) {
+ write_stderr(_("%s: invalid value '%s' for -t\n"),
+ progname, optarg);
+ exit(1);
+ }
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 e9b5c8a448..97a4ac5f44 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -296,6 +296,7 @@ main(int argc, char **argv)
int plainText = 0;
ArchiveFormat archiveFormat = archUnknown;
ArchiveMode archiveMode;
+ char *strtol_endptr = NULL;
static DumpOptions dopt;
@@ -438,7 +439,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'n': /* include schema(s) */
@@ -504,8 +510,8 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ compressLevel = strtol(optarg, &strtol_endptr, 10);
+ if (compressLevel < 0 || compressLevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
write_msg(NULL, "compression level must be in range 0..9\n");
exit_nicely(1);
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index ddd79429b4..dac5f7b2de 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -73,6 +73,7 @@ main(int argc, char **argv)
static int use_setsessauth = 0;
static int no_security_labels = 0;
static int strict_names = 0;
+ char *strtol_endptr = NULL;
struct option cmdopts[] = {
{"clean", 0, NULL, 'c'},
@@ -177,7 +178,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'l': /* Dump the TOC summary */
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 5007ce53cf..b4acbdc727 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[])
int os_user_effective_id;
FILE *fp;
char **filename;
+ char *strtol_endptr;
time_t run_time = time(NULL);
user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -167,7 +168,7 @@ 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)
+ if ((old_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid old port number\n");
exit(1);
@@ -175,7 +176,7 @@ parseCommandLine(int argc, char *argv[])
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
+ if ((new_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid new port number\n");
exit(1);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ae3624721e..e34786747b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3680,6 +3680,8 @@ main(int argc, char **argv)
PGresult *res;
char *env;
+ char *strtol_endptr;
+
progname = get_progname(argv[0]);
if (argc > 1)
@@ -3737,8 +3739,8 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0 || nclients > MAXCLIENTS)
+ nclients = strtol(optarg, &strtol_endptr, 10);
+ if (nclients <= 0 || nclients > MAXCLIENTS || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of clients: \"%s\"\n",
optarg);
@@ -3765,8 +3767,8 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ nthreads = strtol(optarg, &strtol_endptr, 10);
+ if (nthreads <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of threads: \"%s\"\n",
optarg);
@@ -3791,8 +3793,8 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ scale = strtol(optarg, &strtol_endptr, 10);
+ if (scale <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg);
exit(1);
@@ -3805,8 +3807,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ nxacts = strtol(optarg, &strtol_endptr, 10);
+ if (nxacts <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of transactions: \"%s\"\n",
optarg);
@@ -3820,8 +3822,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- duration = atoi(optarg);
- if (duration <= 0)
+ duration = strtol(optarg, &strtol_endptr, 10);
+ if (duration <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid duration: \"%s\"\n", optarg);
exit(1);
@@ -3887,8 +3889,8 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ fillfactor = strtol(optarg, &strtol_endptr, 10);
+ if (fillfactor < 10 || fillfactor > 100 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg);
exit(1);
@@ -3913,8 +3915,8 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ progress = strtol(optarg, &strtol_endptr, 10);
+ if (progress <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid thread progress delay: \"%s\"\n",
optarg);
@@ -3975,8 +3977,8 @@ main(int argc, char **argv)
break;
case 5:
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ agg_interval = strtol(optarg, &strtol_endptr, 10);
+ if (agg_interval <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n",
optarg);
--
2.11.0
0001-Port-most-calls-of-atoi-optarg-to-strcol.patchtext/x-patch; charset=UTF-8; name=0001-Port-most-calls-of-atoi-optarg-to-strcol.patchDownload
From da448337bd050518323b45fd5d2929d04b552802 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet <pierre.ducroquet@people-doc.com>
Date: Tue, 18 Apr 2017 09:40:40 +0200
Subject: [PATCH] Port most calls of atoi(optarg) to strcol
atoi does not allow any kind of data check. If the input data is invalid,
the result will be silently truncated to the valid part of input, or just
0 if no digit was found at the beginning of input.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++++++-----
src/bin/pg_basebackup/pg_receivewal.c | 11 ++++++-----
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++++++-----
src/bin/pg_ctl/pg_ctl.c | 8 +++++++-
src/bin/pg_dump/pg_dump.c | 12 +++++++++---
src/bin/pg_dump/pg_restore.c | 8 +++++++-
src/bin/pg_upgrade/option.c | 5 +++--
src/bin/pgbench/pgbench.c | 34 ++++++++++++++++++----------------
8 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..34884fce4f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 1a9fe81be1..f470406678 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -479,6 +479,7 @@ main(int argc, char **argv)
int c;
int option_index;
char *db_name;
+ char *strtol_endptr = NULL;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -513,7 +514,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if ((strtol(optarg, &strtol_endptr, 10) <= 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -531,8 +532,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
@@ -549,8 +550,8 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 6b081bd737..cd69a6fba6 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -706,6 +706,7 @@ main(int argc, char **argv)
uint32 hi,
lo;
char *db_name;
+ char *strtol_endptr;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -735,8 +736,8 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ fsync_interval = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (fsync_interval < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid fsync interval \"%s\"\n"),
progname, optarg);
@@ -757,7 +758,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if (strtol(optarg, &strtol_endptr, 10) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -819,8 +820,8 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (standby_message_timeout < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index c63819b88b..65191a5319 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2177,6 +2177,7 @@ main(int argc, char **argv)
};
char *env_wait;
+ char *strtol_endptr;
int option_index;
int c;
pgpid_t killproc = 0;
@@ -2307,7 +2308,12 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ wait_seconds = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg)) {
+ write_stderr(_("%s: invalid value '%s' for -t\n"),
+ progname, optarg);
+ exit(1);
+ }
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 e9b5c8a448..97a4ac5f44 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -296,6 +296,7 @@ main(int argc, char **argv)
int plainText = 0;
ArchiveFormat archiveFormat = archUnknown;
ArchiveMode archiveMode;
+ char *strtol_endptr = NULL;
static DumpOptions dopt;
@@ -438,7 +439,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'n': /* include schema(s) */
@@ -504,8 +510,8 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ compressLevel = strtol(optarg, &strtol_endptr, 10);
+ if (compressLevel < 0 || compressLevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
write_msg(NULL, "compression level must be in range 0..9\n");
exit_nicely(1);
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index ddd79429b4..dac5f7b2de 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -73,6 +73,7 @@ main(int argc, char **argv)
static int use_setsessauth = 0;
static int no_security_labels = 0;
static int strict_names = 0;
+ char *strtol_endptr = NULL;
struct option cmdopts[] = {
{"clean", 0, NULL, 'c'},
@@ -177,7 +178,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'l': /* Dump the TOC summary */
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 5007ce53cf..b4acbdc727 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[])
int os_user_effective_id;
FILE *fp;
char **filename;
+ char *strtol_endptr;
time_t run_time = time(NULL);
user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -167,7 +168,7 @@ 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)
+ if ((old_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid old port number\n");
exit(1);
@@ -175,7 +176,7 @@ parseCommandLine(int argc, char *argv[])
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
+ if ((new_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid new port number\n");
exit(1);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ae3624721e..e34786747b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3680,6 +3680,8 @@ main(int argc, char **argv)
PGresult *res;
char *env;
+ char *strtol_endptr;
+
progname = get_progname(argv[0]);
if (argc > 1)
@@ -3737,8 +3739,8 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0 || nclients > MAXCLIENTS)
+ nclients = strtol(optarg, &strtol_endptr, 10);
+ if (nclients <= 0 || nclients > MAXCLIENTS || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of clients: \"%s\"\n",
optarg);
@@ -3765,8 +3767,8 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ nthreads = strtol(optarg, &strtol_endptr, 10);
+ if (nthreads <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of threads: \"%s\"\n",
optarg);
@@ -3791,8 +3793,8 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ scale = strtol(optarg, &strtol_endptr, 10);
+ if (scale <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg);
exit(1);
@@ -3805,8 +3807,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ nxacts = strtol(optarg, &strtol_endptr, 10);
+ if (nxacts <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of transactions: \"%s\"\n",
optarg);
@@ -3820,8 +3822,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- duration = atoi(optarg);
- if (duration <= 0)
+ duration = strtol(optarg, &strtol_endptr, 10);
+ if (duration <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid duration: \"%s\"\n", optarg);
exit(1);
@@ -3887,8 +3889,8 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ fillfactor = strtol(optarg, &strtol_endptr, 10);
+ if (fillfactor < 10 || fillfactor > 100 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg);
exit(1);
@@ -3913,8 +3915,8 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ progress = strtol(optarg, &strtol_endptr, 10);
+ if (progress <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid thread progress delay: \"%s\"\n",
optarg);
@@ -3975,8 +3977,8 @@ main(int argc, char **argv)
break;
case 5:
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ agg_interval = strtol(optarg, &strtol_endptr, 10);
+ if (agg_interval <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n",
optarg);
--
2.11.0
On Sat, Apr 22, 2017 at 11:12 PM, Pierre Ducroquet <p.psql@pinaraf.info> wrote:
Following your advice, I went through the source tree and cleaned up most
instances of that pattern.
I have attached the corresponding patch to this mail.
If you think this patch is indeed interesting, what would be the next way to
have it reviewed ?
Here are the general guidelines about patch submission:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
And the best thing would be to register it to the next commit fest so
as it does not get lost:
https://commitfest.postgresql.org/
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Saturday, April 22, 2017 11:31:58 PM CEST Michael Paquier wrote:
On Sat, Apr 22, 2017 at 11:12 PM, Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
Following your advice, I went through the source tree and cleaned up most
instances of that pattern.
I have attached the corresponding patch to this mail.
If you think this patch is indeed interesting, what would be the next way
to have it reviewed ?Here are the general guidelines about patch submission:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
And the best thing would be to register it to the next commit fest so
as it does not get lost:
https://commitfest.postgresql.org/
Thank you, I added an entry in the next commit fest.
On Sat, Apr 22, 2017 at 12:46 PM, Pierre Ducroquet <p.psql@pinaraf.info> wrote:
Here are the general guidelines about patch submission:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
And the best thing would be to register it to the next commit fest so
as it does not get lost:
https://commitfest.postgresql.org/Thank you, I added an entry in the next commit fest.
Isn't (strtol_endptr != optarg + strlen(optarg))) just a really
inefficient way of writing (strtol_endptr != '\0')?
I would be inclined to write tests like if (standby_message_timeout <
0 || strtol_endptr != optarg + strlen(optarg)) in the other order;
that is, test strtol_endptr first, and only test the other conditions
if that test passes.
I would probably also just use endptr rather than strtol_endptr, for brevity.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Pierre,
I tried to apply your patch to test it (though reading Robert's last comment it seems we wish to have it adjusted before committing)... but in any case I was not able to apply your patch to the tip of the master branch (my git apply failed). I'm setting this to Waiting On Author for a new patch, and I also agree with Robert that the test can be simpler and can go in the other order. If you don't have time to make another patch, let me know, I may be able to make one.
Thanks!
Ryan
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
I tried to apply your patch to test it (though reading Robert's last comment it seems we wish to have it adjusted before committing)... but in any case I was not able to apply your patch to the tip of the master branch (my git apply failed). I'm setting this to Waiting On Author for a new patch, and I also agree with Robert that the test can be simpler and can go in the other order. If you don't have time to make another patch, let me know, I may be able to make one.
git is unhappy even if forcibly applied with patch -p1. You should
check for whitespaces at the same time:
$ git diff --check
src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
+ char *strtol_endptr = NULL
And there are more than this one.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05 Jul 2017, at 08:32, Michael Paquier <michael.paquier@gmail.com> wrote:
On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
I tried to apply your patch to test it (though reading Robert's last comment it seems we wish to have it adjusted before committing)... but in any case I was not able to apply your patch to the tip of the master branch (my git apply failed). I'm setting this to Waiting On Author for a new patch, and I also agree with Robert that the test can be simpler and can go in the other order. If you don't have time to make another patch, let me know, I may be able to make one.
git is unhappy even if forcibly applied with patch -p1. You should
check for whitespaces at the same time:
$ git diff --check
src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
+ char *strtol_endptr = NULL
And there are more than this one.
Like Michael said above, this patch no longer applies and have some whitespace
issues. The conflicts in applying are quite trivial though, so it should be
easy to create a new version. Do you plan to work on this during this
Commitfest so we can move this patch forward?
cheers ./daniel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
On 05 Jul 2017, at 08:32, Michael Paquier <michael.paquier@gmail.com>
wrote:>
On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:I tried to apply your patch to test it (though reading Robert's last
comment it seems we wish to have it adjusted before committing)... but
in any case I was not able to apply your patch to the tip of the master
branch (my git apply failed). I'm setting this to Waiting On Author for
a new patch, and I also agree with Robert that the test can be simpler
and can go in the other order. If you don't have time to make another
patch, let me know, I may be able to make one.>git is unhappy even if forcibly applied with patch -p1. You should
check for whitespaces at the same time:
$ git diff --check
src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
+ char *strtol_endptr = NULL
And there are more than this one.Like Michael said above, this patch no longer applies and have some
whitespace issues. The conflicts in applying are quite trivial though, so
it should be easy to create a new version. Do you plan to work on this
during this Commitfest so we can move this patch forward?
Yes, my bad. Attached to this email is the new version, that now applies on
master.
Sorry for the delay :(
Attachments:
0001-Port-most-calls-of-atoi-optarg-to-strcol.patchtext/x-patch; charset=UTF-8; name=0001-Port-most-calls-of-atoi-optarg-to-strcol.patchDownload
From 7efc1573c1bcc07c0eaa80912e6e035f2e0d203d Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet <p.psql@pinaraf.info>
Date: Wed, 13 Sep 2017 19:51:09 +0200
Subject: [PATCH] Port most calls of atoi(optarg) to strcol
atoi does not allow any kind of data check. If the input data is invalid,
the result will be silently truncated to the valid part of input, or just
0 if no digit was found at the beginning of input.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++++++-----
src/bin/pg_basebackup/pg_receivewal.c | 11 ++++++-----
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++++++-----
src/bin/pg_ctl/pg_ctl.c | 9 ++++++++-
src/bin/pg_dump/pg_dump.c | 12 +++++++++---
src/bin/pg_dump/pg_restore.c | 8 +++++++-
src/bin/pg_upgrade/option.c | 5 +++--
src/bin/pgbench/pgbench.c | 33 +++++++++++++++++----------------
8 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 51509d150e..b51b62cc21 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2072,6 +2072,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2212,8 +2213,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2251,8 +2252,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 710a33ab4d..c1651961b5 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -494,6 +494,7 @@ main(int argc, char **argv)
int c;
int option_index;
char *db_name;
+ char *strtol_endptr = NULL;
uint32 hi, lo;
progname = get_progname(argv[0]);
@@ -529,7 +530,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if ((strtol(optarg, &strtol_endptr, 10) <= 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -547,8 +548,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
@@ -575,8 +576,8 @@ main(int argc, char **argv)
verbose++;
break;
case 'Z':
- compresslevel = atoi(optarg);
- if (compresslevel < 0 || compresslevel > 9)
+ compresslevel = strtol(optarg, &strtol_endptr, 10);
+ if (compresslevel < 0 || compresslevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 6811a55e76..bde45b2650 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -706,6 +706,7 @@ main(int argc, char **argv)
uint32 hi,
lo;
char *db_name;
+ char *strtol_endptr = NULL;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -735,8 +736,8 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ fsync_interval = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (fsync_interval < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid fsync interval \"%s\"\n"),
progname, optarg);
@@ -757,7 +758,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ if (strtol(optarg, &strtol_endptr, 10) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
progname, optarg);
@@ -819,8 +820,8 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
break;
case 's':
- standby_message_timeout = atoi(optarg) * 1000;
- if (standby_message_timeout < 0)
+ standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+ if (standby_message_timeout < 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 4e02c4cea1..2190412293 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2100,6 +2100,7 @@ main(int argc, char **argv)
};
char *env_wait;
+ char *strtol_endptr = NULL;
int option_index;
int c;
pgpid_t killproc = 0;
@@ -2231,7 +2232,13 @@ main(int argc, char **argv)
#endif
break;
case 't':
- wait_seconds = atoi(optarg);
+ wait_seconds = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_stderr(_("%s: invalid value '%s' for -t\n"),
+ progname, optarg);
+ exit(1);
+ }
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 75f08cd792..ddbf742878 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -290,6 +290,7 @@ main(int argc, char **argv)
const char *dumpencoding = NULL;
const char *dumpsnapshot = NULL;
char *use_role = NULL;
+ char *strtol_endptr = NULL;
int numWorkers = 1;
trivalue prompt_password = TRI_DEFAULT;
int compressLevel = -1;
@@ -441,7 +442,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of dump jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'n': /* include schema(s) */
@@ -507,8 +513,8 @@ main(int argc, char **argv)
break;
case 'Z': /* Compression Level */
- compressLevel = atoi(optarg);
- if (compressLevel < 0 || compressLevel > 9)
+ compressLevel = strtol(optarg, &strtol_endptr, 10);
+ if (compressLevel < 0 || compressLevel > 9 || strtol_endptr != optarg + strlen(optarg))
{
write_msg(NULL, "compression level must be in range 0..9\n");
exit_nicely(1);
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 860a211a3c..e759a6e960 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -65,6 +65,7 @@ main(int argc, char **argv)
int numWorkers = 1;
Archive *AH;
char *inputFileSpec;
+ char *strtol_endptr = NULL;
static int disable_triggers = 0;
static int enable_row_security = 0;
static int if_exists = 0;
@@ -181,7 +182,12 @@ main(int argc, char **argv)
break;
case 'j': /* number of restore jobs */
- numWorkers = atoi(optarg);
+ numWorkers = strtol(optarg, &strtol_endptr, 10);
+ if (strtol_endptr != optarg + strlen(optarg))
+ {
+ write_msg(NULL, "invalid number of jobs\n");
+ exit_nicely(1);
+ }
break;
case 'l': /* Dump the TOC summary */
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index c74eb25e18..678b5a8bfa 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[])
int os_user_effective_id;
FILE *fp;
char **filename;
+ char *strtol_endptr = NULL;
time_t run_time = time(NULL);
user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -167,7 +168,7 @@ 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)
+ if ((old_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid old port number\n");
exit(1);
@@ -175,7 +176,7 @@ parseCommandLine(int argc, char *argv[])
break;
case 'P':
- if ((new_cluster.port = atoi(optarg)) <= 0)
+ if ((new_cluster.port = strtol(optarg, &strtol_endptr, 10)) <= 0 || strtol_endptr != optarg + strlen(optarg))
{
pg_fatal("invalid new port number\n");
exit(1);
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c971..27746bcd89 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3708,6 +3708,7 @@ main(int argc, char **argv)
PGconn *con;
PGresult *res;
char *env;
+ char *strtol_endptr;
progname = get_progname(argv[0]);
@@ -3766,8 +3767,8 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0 || nclients > MAXCLIENTS)
+ nclients = strtol(optarg, &strtol_endptr, 10);
+ if (nclients <= 0 || nclients > MAXCLIENTS || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of clients: \"%s\"\n",
optarg);
@@ -3794,8 +3795,8 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ nthreads = strtol(optarg, &strtol_endptr, 10);
+ if (nthreads <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of threads: \"%s\"\n",
optarg);
@@ -3820,8 +3821,8 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ scale = strtol(optarg, &strtol_endptr, 10);
+ if (scale <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg);
exit(1);
@@ -3834,8 +3835,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ nxacts = strtol(optarg, &strtol_endptr, 10);
+ if (nxacts <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of transactions: \"%s\"\n",
optarg);
@@ -3849,8 +3850,8 @@ main(int argc, char **argv)
fprintf(stderr, "specify either a number of transactions (-t) or a duration (-T), not both\n");
exit(1);
}
- duration = atoi(optarg);
- if (duration <= 0)
+ duration = strtol(optarg, &strtol_endptr, 10);
+ if (duration <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid duration: \"%s\"\n", optarg);
exit(1);
@@ -3916,8 +3917,8 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ fillfactor = strtol(optarg, &strtol_endptr, 10);
+ if (fillfactor < 10 || fillfactor > 100 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg);
exit(1);
@@ -3937,8 +3938,8 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ progress = strtol(optarg, &strtol_endptr, 10);
+ if (progress <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid thread progress delay: \"%s\"\n",
optarg);
@@ -3999,8 +4000,8 @@ main(int argc, char **argv)
break;
case 5:
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ agg_interval = strtol(optarg, &strtol_endptr, 10);
+ if (agg_interval <= 0 || strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n",
optarg);
--
2.14.1
Great, thanks Pierre!
I don't have a chance to try the patch tonight, but I will on the weekend
if no one else beats me to it.
On Wed, Sep 13, 2017 at 12:53 PM Pierre Ducroquet <p.psql@pinaraf.info>
wrote:
Show quoted text
On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
On 05 Jul 2017, at 08:32, Michael Paquier <michael.paquier@gmail.com>
wrote:>
On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy <ryanfmurphy@gmail.com>wrote:
I tried to apply your patch to test it (though reading Robert's last
comment it seems we wish to have it adjusted before committing)... but
in any case I was not able to apply your patch to the tip of themaster
branch (my git apply failed). I'm setting this to Waiting On Author
for
a new patch, and I also agree with Robert that the test can be simpler
and can go in the other order. If you don't have time to make another
patch, let me know, I may be able to make one.>git is unhappy even if forcibly applied with patch -p1. You should
check for whitespaces at the same time:
$ git diff --check
src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
+ char *strtol_endptr = NULL
And there are more than this one.Like Michael said above, this patch no longer applies and have some
whitespace issues. The conflicts in applying are quite trivial though,so
it should be easy to create a new version. Do you plan to work on this
during this Commitfest so we can move this patch forward?Yes, my bad. Attached to this email is the new version, that now applies on
master.Sorry for the delay :(
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
I applied this patch via patch -p1. (Had an issue using git apply, but apparently that's common?)
All tests passed normally. Ran make check, make installcheck, and make installcheck-world.
Looked thru the diffs and it looks fine to me.
Changing a lot of a integer/long arguments that were being read directly via atoi / atol
to be read as strings first, to give better error handling.
This looks good to go to me. Reviewing this as "Ready for Committer".
Thanks for the patch, Pierre!
Ryan
The new status of this patch is: Ready for Committer
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Ryan Murphy <ryanfmurphy@gmail.com> writes:
Looked thru the diffs and it looks fine to me.
Changing a lot of a integer/long arguments that were being read directly via atoi / atol
to be read as strings first, to give better error handling.This looks good to go to me. Reviewing this as "Ready for Committer".
Thanks for the patch, Pierre!
I took a quick look through this and had a few thoughts:
* If we're taking the trouble to sanity-check the input, I think we should
also check for ERANGE (i.e., overflow).
* I concur with Robert that you might as well just test for "*endptr != '\0'"
instead of adding a strlen() call.
* Replicating fiddly little code sequences like this all over the place
makes me itch. There's too much chance to get it wrong, and even more
risk of someone taking shortcuts. It has to be not much more complicated
than using atoi(), or we'll just be doing this exercise again in a few
years. So I'm thinking we need to introduce a convenience function,
perhaps located in src/common/, or else src/fe_utils/.
* Changing variables from int to long int is likely to have unpleasant
consequences on platforms where they're different; or at least, I don't
particularly feel like auditing the patch to ensure that that's not a
problem anywhere. So I think we should not do that but just make the
convenience function return int, with a suitable overflow test for that
result size. There's existing logic you can copy in
src/backend/nodes/read.c:
errno = 0;
val = strtol(token, &endptr, 10);
if (endptr != token + length || errno == ERANGE
#ifdef HAVE_LONG_INT_64
/* if long > 32 bits, check for overflow of int4 */
|| val != (long) ((int32) val)
#endif
)
... bad integer ...
If there are places where we actually do want a long result, we
could have two convenience functions, one returning int and one long.
The exact form of the convenience function is up for grabs, but one
way we could do it is
bool pg_int_convert(const char *str, int *value)
(where a true result indicates a valid integer value).
Then typical usage would be like
if (!pg_int_convert(optarg, &compresslevel) ||
compresslevel < 0 || compresslevel > 9)
... complain-and-exit ...
There might be something to be said for folding the range checks
into the convenience function:
bool pg_int_convert(const char *str, int min, int max, int *value)
if (!pg_int_convert(optarg, 0, 9, &compresslevel))
... complain-and-exit ...
That way you could protect code like
standby_message_timeout = atoi(optarg) * 1000;
fairly easily:
if (!pg_int_convert(optarg, 0, INT_MAX/1000,
&standby_message_timeout))
... complain-and-exit ...
standby_message_timeout *= 1000;
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
Ryan Murphy <ryanfmurphy@gmail.com> writes:
Looked thru the diffs and it looks fine to me.
Changing a lot of a integer/long arguments that were being read directly
via atoi / atol to be read as strings first, to give better error
handling.This looks good to go to me. Reviewing this as "Ready for Committer".
Thanks for the patch, Pierre!I took a quick look through this and had a few thoughts:
I agree with your suggestions, I will try to produce a new patch before the
end of the week.
* If we're taking the trouble to sanity-check the input, I think we should
also check for ERANGE (i.e., overflow).* I concur with Robert that you might as well just test for "*endptr !=
'\0'" instead of adding a strlen() call.* Replicating fiddly little code sequences like this all over the place
makes me itch. There's too much chance to get it wrong, and even more
risk of someone taking shortcuts. It has to be not much more complicated
than using atoi(), or we'll just be doing this exercise again in a few
years. So I'm thinking we need to introduce a convenience function,
perhaps located in src/common/, or else src/fe_utils/.* Changing variables from int to long int is likely to have unpleasant
consequences on platforms where they're different; or at least, I don't
particularly feel like auditing the patch to ensure that that's not a
problem anywhere. So I think we should not do that but just make the
convenience function return int, with a suitable overflow test for that
result size. There's existing logic you can copy in
src/backend/nodes/read.c:errno = 0;
val = strtol(token, &endptr, 10);
if (endptr != token + length || errno == ERANGE
#ifdef HAVE_LONG_INT_64
/* if long > 32 bits, check for overflow of int4 */|| val != (long) ((int32) val)
#endif
)
... bad integer ...If there are places where we actually do want a long result, we
could have two convenience functions, one returning int and one long.The exact form of the convenience function is up for grabs, but one
way we could do it isbool pg_int_convert(const char *str, int *value)
(where a true result indicates a valid integer value).
Then typical usage would be likeif (!pg_int_convert(optarg, &compresslevel) ||
compresslevel < 0 || compresslevel > 9)
... complain-and-exit ...There might be something to be said for folding the range checks
into the convenience function:bool pg_int_convert(const char *str, int min, int max, int *value)
if (!pg_int_convert(optarg, 0, 9, &compresslevel))
... complain-and-exit ...That way you could protect code like
standby_message_timeout = atoi(optarg) * 1000;
fairly easily:
if (!pg_int_convert(optarg, 0, INT_MAX/1000,
&standby_message_timeout))
... complain-and-exit ...
standby_message_timeout *= 1000;regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 18 Sep 2017, at 23:18, Pierre Ducroquet <p.psql@pinaraf.info> wrote:
On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
Ryan Murphy <ryanfmurphy@gmail.com> writes:
Looked thru the diffs and it looks fine to me.
Changing a lot of a integer/long arguments that were being read directly
via atoi / atol to be read as strings first, to give better error
handling.This looks good to go to me. Reviewing this as "Ready for Committer".
Thanks for the patch, Pierre!I took a quick look through this and had a few thoughts:
I agree with your suggestions, I will try to produce a new patch before the
end of the week.
This patch has been marked as Returned with Feedback as no new version has been
submitted. Please re-submit the new version of this patch to the next commit-
fest when it’s ready.
cheers ./daniel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-Sep-18, Pierre Ducroquet wrote:
On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
I took a quick look through this and had a few thoughts:
I agree with your suggestions, I will try to produce a new patch before the
end of the week.
Hi Pierre. Are you updating this patch soon?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services