Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Daniel Gustafsson]

> On 18 Sep 2017, at 23:18, Pierre Ducroquet  wrote:
> 
> On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
>> Ryan Murphy  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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
> Ryan Murphy  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, , 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 < 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, ))
> ... 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,
> _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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Tom Lane]

Ryan Murphy  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, , 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 < 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, ))
... 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,
_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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

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 
wrote:

> On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
> > > On 05 Jul 2017, at 08:32, Michael Paquier 
> > > wrote:>
> > > On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy 
> 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 :(
>

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
> > On 05 Jul 2017, at 08:32, Michael Paquier 
> > wrote:> 
> > On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy  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 :(
>From 7efc1573c1bcc07c0eaa80912e6e035f2e0d203d Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet 
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, _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, _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, _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;
-

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Daniel Gustafsson]

> On 05 Jul 2017, at 08:32, Michael Paquier  wrote:
> 
> On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy  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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Michael Paquier]

On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy  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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Ryan Murphy]

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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Robert Haas]

On Sat, Apr 22, 2017 at 12:46 PM, Pierre Ducroquet  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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Saturday, April 22, 2017 11:31:58 PM CEST Michael Paquier wrote:
> On Sat, Apr 22, 2017 at 11:12 PM, Pierre Ducroquet  
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.


signature.asc
Description: This is a digitally signed message part.

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Michael Paquier]

On Sat, Apr 22, 2017 at 11:12 PM, Pierre Ducroquet  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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote:
> On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet 
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 
> >
> > 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
From da448337bd050518323b45fd5d2929d04b552802 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet 
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, _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, _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, _endptr, 10) <= 0) || (strtol_endptr != optarg + strlen(optarg)))
 {
 	fprintf(stderr, _("%s: invalid port number \"%s\"\n"),
 			progname, optarg);
@@ -531,8 +532,8 @@ main(int 

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote:
> On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet  
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 
> > 
> > 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.

signature.asc
Description: This is a digitally signed message part.

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Robert Haas]

On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet  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 
> 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

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Pierre Ducroquet]

On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
> On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet  
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.

signature.asc
Description: This is a digitally signed message part.

Re: [HACKERS] Small patch for pg_basebackup argument parsing

[Michael Paquier]

On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet  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