On 23/10/2022 06:57, René Scharfe wrote:
Am 22.10.22 um 18:51 schrieb Junio C Hamano:
René Scharfe <l.s.r@xxxxxx> writes:
+ if (max < 0)
+ BUG("max must be a positive integer");
In parse_signed(), would we expect to accept end-user input that is
a negative integer? We must. Otherwise we would not be calling a
"signed" parser. Now, are there cases where the valid value range
is bounded by a negative integer at the top? No current callers may
pass such a value, but is it reasonable to add such a new constraints
to an existing API function?
Hmm, if minimum and maximum are not symmetric, then we need to supply
both, don't we?
Ah, thanks for injecting doze of sanity---I totally missed that the
bound was about the absolute value, so we can say "this is signed,
and the allowed values are (-3, -2, -1, 0, 1, 2, 3). If so, then the
"reject negative max" in the posted patch is not a problem as I said
above. I somehow thought that giving -1 as "max" would allow callers
to say "non-negative numbers are not allowed". But that is not what
is going on.
Right, currently the value of `max` is used to check the absolute value,
i.e. it imposes a limit in both the positive and negative direction.
Allowing callers to specify both lower and uppoer bounds so that
they can say "the allowed values are (-1, 0, 1, 2, 3)", while it
might make it more useful, is a separate new feature development and
outside the scope of "let's tighten the parsing of end user input"
Phillip has here.
Allowing arbitrary limits in both directions might be a new feature, but
it's required if we want to support the full range of values. E.g. on
my system INT_MAX is 2147483647 and INT_MIN is -2147483647-1. Currently
git_parse_int() rejects INT_MIN as out of range.
In my eyes the assumption that a single limit can be used to check both
directions of the signed number line is flawed and caused the undefined
behavior. Dropping it avoids tricky calculations that try to infer the
lower limit somehow and opens the full range of values to us.
That said, I'm not sure how useful the values INT_MIN, INT64_MIN and
SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually
are. But doing the checks properly requires separate min and max values.
I'm happy to go either way, while I agree passing separate limits to
allow INT_MIN is technically correct I'm not sure anyone has complained
that the current code is too restrictive.
Best Wishes
Phillip
René
[*] Perhaps git_parse_ssize_t() should reject values less than -1; only
that single negative number is needed to represent errors or unlimited.
