Re: [PATCH] show_ident_date: fix always-false conditional

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 7, 2014 at 12:15 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Thu, Mar 06, 2014 at 08:35:24PM -0500, Eric Sunshine wrote:
>
>> 1dca155fe3fa (log: handle integer overflow in timestamps, 2014-02-24)
>> assigns the result of strtol() to an 'int' and then checks it against
>> LONG_MIN and LONG_MAX, indicating underflow or overflow, even though
>> 'int' may not be large enough to represent those values.
>>
>> On Mac, the compiler complains:
>>
>>     warning: comparison of constant 9223372036854775807 with
>>       expression of type 'int' is always false
>>       [-Wtautological-constant-out-of-range-compare]
>>       if (<<tz == LONG_MAX>> || tz == LONG_MIN)
>>
>> Similarly for the LONG_MIN case. Fix this.
>
> Yeah, this is definitely a potential bug. When I added the overflow
> check, I blindly assumed that the existing code was at least using a
> sufficiently large type to store the result of strtol, but it's not.
>
> I don't think your fix catches all overflow, though:
>
>> +     else if (ident->tz_begin && ident->tz_end) {
>> +             errno = 0;
>> +             tz = strtol(ident->tz_begin, NULL, 10);
>> +             if (errno)
>
> Errno will trigger if we overflowed a "long", but then we assign the
> result into an int, possibly truncating the result.
>
>> Alternately, the result of strtol() could be assigned temporarily to a
>> 'long', compared against LONG_MIN and LONG_MAX, and then assigned to the
>> 'int' "tz" variable.
>
> That catches overflow from strtol, but we'd then truncate when we pass
> it as an int to show_date.
>
> I think we want this instead:

Makes sense.

Acked-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>

> -- >8 --
> Subject: show_ident_date: fix tz range check
>
> Commit 1dca155fe3fa (log: handle integer overflow in
> timestamps, 2014-02-24) tried to catch integer overflow
> coming from strtol() on the timezone field by comparing against
> LONG_MIN/LONG_MAX. However, the intermediate "tz" variable
> is an "int", which means it can never be LONG_MAX on LP64
> systems; we would truncate the output from strtol before the
> comparison.
>
> Clang's -Wtautological-constant-out-of-range-compare notices
> this and rightly complains.
>
> Let's instead store the result of strtol in a long, and then
> compare it against INT_MIN/INT_MAX. This will catch overflow
> from strtol, and also overflow when we pass the result as an
> int to show_date.
>
> Reported-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
>  pretty.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 3b811ed..6e266dd 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -397,7 +397,7 @@ static const char *show_ident_date(const struct ident_split *ident,
>                                    enum date_mode mode)
>  {
>         unsigned long date = 0;
> -       int tz = 0;
> +       long tz = 0;
>
>         if (ident->date_begin && ident->date_end)
>                 date = strtoul(ident->date_begin, NULL, 10);
> @@ -406,7 +406,7 @@ static const char *show_ident_date(const struct ident_split *ident,
>         else {
>                 if (ident->tz_begin && ident->tz_end)
>                         tz = strtol(ident->tz_begin, NULL, 10);
> -               if (tz == LONG_MAX || tz == LONG_MIN)
> +               if (tz >= INT_MAX || tz <= INT_MIN)
>                         tz = 0;
>         }
>         return show_date(date, tz, mode);
> --
> 1.8.5.2.500.g8060133
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]