Thread: [HACKERS] 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
Attachment
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
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
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
Attachment
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
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
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 beforecommitting)... 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 simplerand 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
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 beforecommitting)... 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 simplerand 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
> 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 beforecommitting)... 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 simplerand 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 :( -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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:
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 :(
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 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 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