Re: [HACKERS] Small patch for pg_basebackup argument parsing - Mailing list 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