Re: [PATCH] show_ident_date: fix always-false conditional
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 sunsh...@sunshineco.com
Signed-off-by: Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] show_ident_date: fix always-false conditional
On Fri, Mar 7, 2014 at 12:15 PM, Jeff King p...@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 sunsh...@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 sunsh...@sunshineco.com
Signed-off-by: Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
