Re: [PATCH 4/4] config: avoid calling `labs()` on too-large data type
- From
René Scharfe <l.s.r@web.de>
- Date
- Jun 16, 2019, 08:24 UTC
- Message-ID
- <f22599d4-148f-212c-282c-790f5bde1706@web.de>
- In-Reply-To
- <03a68560-8058-7436-6edb-38e276a37190@web.de>
Am 16.06.19 um 08:48 schrieb René Scharfe:
Show 42 quoted lines
> Am 13.06.19 um 13:49 schrieb Johannes Schindelin via GitGitGadget:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> The `labs()` function operates, as the initial `l` suggests, on `long`
>> parameters. However, in `config.c` we tried to use it on values of type
>> `intmax_t`.
>>
>> This problem was found by GCC v9.x.
>>
>> To fix it, let's just "unroll" the function (i.e. negate the value if it
>> is negative).
>
> There's also imaxabs(3).
>
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> config.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index 296a6d9cc4..01c6e9df23 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -869,9 +869,9 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>> errno = EINVAL;
>> return 0;
>> }
>> - uval = labs(val);
>> + uval = val < 0 ? -val : val;
>> uval *= factor;
>> - if (uval > max || labs(val) > uval) {
>> + if (uval > max || (val < 0 ? -val : val) > uval) {
>> errno = ERANGE;
>> return 0;
>> }
>
> So this check uses unsigned arithmetic to find out if the multiplication
> overflows, right? Let's say value is "4G", then val will be 4 and
> factor will be 2^30. Multiplying the two yields 2^32. On a 32-bit
> system this will wrap around to 0, so that's what we get for uval there.
> The range check will then pass unless max is negative, which is wrong.No, this example is wrong. (I need to remember to take baby steps while carrying numbers. o_O)
So value = "5G", then val = 5 and factor = 2^30. After multiplication uval = 2^32 + 2^30, on 32-bit machines this is 2^30 due to wrap-around. Correct so far?
If uval is 2^30, then it's smaller than 2^31, so will pass a check against INT_MAX. val is 5, which is smaller than 2^30, so will pass the second check as well. Makes sense?
That would mean "5G" will overflow on a 32-bit machine, but we won't detect it.
René