Am 16.06.19 um 10:24 schrieb René Scharfe:
> Am 16.06.19 um 08:48 schrieb René Scharfe:
>> Am 13.06.19 um 13:49 schrieb Johannes Schindelin via GitGitGadget:
>>> From: Johannes Schindelin <johannes.schinde...@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.schinde...@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.
Yeah, except that in the real world uintmax_t is 8 bytes wide
everywhere, even on x86 and ARM. So the code should be fine as-is. It
would be in trouble if we introduced bigger units, like T for 2^40 etc.,
though.
Anyway, the code would be easier to read and ready for any units if it
used unsigned_mult_overflows; would have saved me time spent painfully
wading through the math at least. (Hopefully that's just my problem,
though.)
René
