* [PATCH] show_ident_date: fix always-false conditional
@ 2014-03-07 1:35 Eric Sunshine
2014-03-07 17:15 ` Jeff King
0 siblings, 1 reply; 3+ messages in thread
From: Eric Sunshine @ 2014-03-07 1:35 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Jeff King
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.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
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. I chose the 'errno' approach instead because its
dead obvious, even to the most casual reader who hasn't checked the
strtol() man page, that it's handling a conversion failure. However, I
could go either way.
This patch is atop 'next'.
pretty.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index 3b811ed..8903116 100644
--- a/pretty.c
+++ b/pretty.c
@@ -403,10 +403,10 @@ static const char *show_ident_date(const struct ident_split *ident,
date = strtoul(ident->date_begin, NULL, 10);
if (date_overflows(date))
date = 0;
- else {
- if (ident->tz_begin && ident->tz_end)
- tz = strtol(ident->tz_begin, NULL, 10);
- if (tz == LONG_MAX || tz == LONG_MIN)
+ else if (ident->tz_begin && ident->tz_end) {
+ errno = 0;
+ tz = strtol(ident->tz_begin, NULL, 10);
+ if (errno)
tz = 0;
}
return show_date(date, tz, mode);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] show_ident_date: fix always-false conditional
2014-03-07 1:35 [PATCH] show_ident_date: fix always-false conditional Eric Sunshine
@ 2014-03-07 17:15 ` Jeff King
2014-03-07 18:12 ` Eric Sunshine
0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2014-03-07 17:15 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
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:
-- >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@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] show_ident_date: fix always-false conditional
2014-03-07 17:15 ` Jeff King
@ 2014-03-07 18:12 ` Eric Sunshine
0 siblings, 0 replies; 3+ messages in thread
From: Eric Sunshine @ 2014-03-07 18:12 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
On Fri, Mar 7, 2014 at 12:15 PM, Jeff King <peff@peff.net> 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@sunshineco.com>
> -- >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@sunshineco.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-03-07 18:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-07 1:35 [PATCH] show_ident_date: fix always-false conditional Eric Sunshine
2014-03-07 17:15 ` Jeff King
2014-03-07 18:12 ` Eric Sunshine
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.