Re: [HACKERS] Small patch for pg_basebackup argument parsing
> 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
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, &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
Re: [HACKERS] Small patch for pg_basebackup argument parsing
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, &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
Re: [HACKERS] Small patch for pg_basebackup argument parsing
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
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
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, &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_mess
Re: [HACKERS] Small patch for pg_basebackup argument parsing
> 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
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
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
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
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
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
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, &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;
Re: [HACKERS] Small patch for pg_basebackup argument parsing
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
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
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
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