Re: Small patch for pg_basebackup argument parsing

On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote:
> Ryan Murphy <ryanfmurphy(at)gmail(dot)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