Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow
Am 21.10.22 um 20:31 schrieb Junio C Hamano:
Show 37 quoted lines
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> git_parse_signed() checks that the absolute value of the parsed string
>> is less than or equal to a caller supplied maximum value. When
>> calculating the absolute value there is a integer overflow if `val ==
>> INTMAX_MIN`.
>
> It is a problem that is worth looking into.
>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> config.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index b7fb68026d8..aad3e00341d 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>> if (value && *value) {
>> char *end;
>> intmax_t val;
>> - uintmax_t uval;
>> - uintmax_t factor;
>> + intmax_t factor;
>> +
>> + 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?
--- >8 ---
Subject: [PATCH] config: let git_parse_signed() check minimum
git_parse_signed() checks that the absolute value of the parsed string is less than or equal to a caller supplied maximum value. When calculating the absolute value there is a integer overflow if `val == INTMAX_MIN`.
Avoid overflow during sign inversion by supplying the minimum value to the function as well. Make `factor` signed to avoid promoting the division results in the limit check line to unsigned, but check whether it's positive.
Add a new macro, minimum_signed_value_of_type, and use it in the callers of git_parse_signed() to provide the newly required minimum value. It calculates the minimum for a given type using division instead of right shift because the latter is implementation-defined for signed types, and we need an arithmetic right shift here, not a logical one.
Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
git_config_get_expiry_in_days() could use git_parse_int(), but that's
a different topic.
config.c | 28 +++++++++++++++++-----------
git-compat-util.h | 2 ++
2 files changed, 19 insertions(+), 11 deletions(-)
Show changes to 2 files +19 −12
config.c, git-compat-util.h
diff --git a/config.c b/config.c
index cbb5a3bab7..7cf196dc84 100644
--- a/config.c
+++ b/config.c
@@ -1155,26 +1155,24 @@ static uintmax_t get_unit_factor(const char *end)
return 0;
}
-static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
+static int git_parse_signed(const char *value, intmax_t *ret,
+ intmax_t min, intmax_t max)
{
if (value && *value) {
char *end;
intmax_t val;
- uintmax_t uval;
- uintmax_t factor;
+ intmax_t factor;
errno = 0;
val = strtoimax(value, &end, 0);
if (errno == ERANGE)
return 0;
factor = get_unit_factor(end);
- if (!factor) {
+ if (factor < 1) {
errno = EINVAL;
return 0;
}
- uval = val < 0 ? -val : val;
- if (unsigned_mult_overflows(factor, uval) ||
- factor * uval > max) {
+ if (val < min / factor || val > max / factor) {
errno = ERANGE;
return 0;
}
@@ -1218,7 +1216,9 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
static int git_parse_int(const char *value, int *ret)
{
intmax_t tmp;
- if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))
+ if (!git_parse_signed(value, &tmp,
+ minimum_signed_value_of_type(int),
+ maximum_signed_value_of_type(int)))
return 0;
*ret = tmp;
return 1;
@@ -1227,7 +1227,9 @@ static int git_parse_int(const char *value, int *ret)
static int git_parse_int64(const char *value, int64_t *ret)
{
intmax_t tmp;
- if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))
+ if (!git_parse_signed(value, &tmp,
+ minimum_signed_value_of_type(int64_t),
+ maximum_signed_value_of_type(int64_t)))
return 0;
*ret = tmp;
return 1;
@@ -1245,7 +1247,9 @@ int git_parse_ulong(const char *value, unsigned long *ret)
int git_parse_ssize_t(const char *value, ssize_t *ret)
{
intmax_t tmp;
- if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
+ if (!git_parse_signed(value, &tmp,
+ minimum_signed_value_of_type(ssize_t),
+ maximum_signed_value_of_type(ssize_t)))
return 0;
*ret = tmp;
return 1;
@@ -2751,7 +2755,9 @@ int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam
if (git_config_get_string_tmp(key, &expiry_string))
return 1; /* no such thing */
- if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
+ if (git_parse_signed(expiry_string, &days,
+ minimum_signed_value_of_type(int),
+ maximum_signed_value_of_type(int))) {
const int scale = 86400;
*expiry = now - days * scale;
return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index ea53ea4a78..35425c373b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -112,6 +112,8 @@ struct strbuf;
#define bitsizeof(x) (CHAR_BIT * sizeof(x))
+#define minimum_signed_value_of_type(a) \
+ (INTMAX_MIN / ((intmax_t)1 << (bitsizeof(intmax_t) - bitsizeof(a))))
#define maximum_signed_value_of_type(a) \
(INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))
--
2.38.1
