Ryan Murphy <ryanfmur...@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
