Re: [PATCH] show_ident_date: fix always-false conditional
On Fri, Mar 7, 2014 at 12:15 PM, Jeff King 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_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 > -- >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 > Signed-off-by: Jeff King > --- > 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 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_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 Signed-off-by: Jeff King --- 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
[PATCH] show_ident_date: fix always-false conditional
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_MIN) Similarly for the LONG_MIN case. Fix this. Signed-off-by: Eric Sunshine --- 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 -- 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