{"thread":{"id":"36081","subject":"[PATCH] show_ident_date: fix always-false conditional","startedAt":"2014-03-07T01:35:24Z","lastAt":"2014-03-07T18:12:58Z","messageCount":3,"participants":["Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236229","messageId":"1394156124-3953-1-git-send-email-sunshine@sunshineco.com","threadId":"36081","inReplyTo":null,"subject":"[PATCH] show_ident_date: fix always-false conditional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-07T01:35:24Z","receivedAt":"2014-03-07T01:35:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"1dca155fe3fa (log: handle integer overflow in timestamps, 2014-02-24)\nassigns the result of strtol() to an 'int' and then checks it against\nLONG_MIN and LONG_MAX, indicating underflow or overflow, even though\n'int' may not be large enough to represent those values.\n\nOn Mac, the compiler complains:\n\n warning: comparison of constant 9223372036854775807 with\n expression of type 'int' is always false\n [-Wtautological-constant-out-of-range-compare]\n if (<> || tz == LONG_MIN)\n\nSimilarly for the LONG_MIN case. Fix this.\n\nSigned-off-by: Eric Sunshine \n---\n\nAlternately, the result of strtol() could be assigned temporarily to a\n'long', compared against LONG_MIN and LONG_MAX, and then assigned to the\n'int' \"tz\" variable. I chose the 'errno' approach instead because its\ndead obvious, even to the most casual reader who hasn't checked the\nstrtol() man page, that it's handling a conversion failure. However, I\ncould go either way.\n\nThis patch is atop 'next'.\n\n pretty.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3b811ed..8903116 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -403,10 +403,10 @@ static const char *show_ident_date(const struct ident_split *ident,\n \t\tdate = strtoul(ident->date_begin, NULL, 10);\n \tif (date_overflows(date))\n \t\tdate = 0;\n-\telse {\n-\t\tif (ident->tz_begin && ident->tz_end)\n-\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n-\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n+\telse if (ident->tz_begin && ident->tz_end) {\n+\t\terrno = 0;\n+\t\ttz = strtol(ident->tz_begin, NULL, 10);\n+\t\tif (errno)\n \t\t\ttz = 0;\n \t}\n \treturn show_date(date, tz, mode);\n-- \n1.8.3.2\n"},{"id":"236263","messageId":"20140307171501.GA23587@sigill.intra.peff.net","threadId":"36081","inReplyTo":"1394156124-3953-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] show_ident_date: fix always-false conditional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-07T17:15:01Z","receivedAt":"2014-03-07T17:15:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2014 at 08:35:24PM -0500, Eric Sunshine wrote:\n\n> 1dca155fe3fa (log: handle integer overflow in timestamps, 2014-02-24)\n> assigns the result of strtol() to an 'int' and then checks it against\n> LONG_MIN and LONG_MAX, indicating underflow or overflow, even though\n> 'int' may not be large enough to represent those values.\n> \n> On Mac, the compiler complains:\n> \n> warning: comparison of constant 9223372036854775807 with\n> expression of type 'int' is always false\n> [-Wtautological-constant-out-of-range-compare]\n> if (<> || tz == LONG_MIN)\n> \n> Similarly for the LONG_MIN case. Fix this.\n\nYeah, this is definitely a potential bug. When I added the overflow\ncheck, I blindly assumed that the existing code was at least using a\nsufficiently large type to store the result of strtol, but it's not.\n\nI don't think your fix catches all overflow, though:\n\n> +\telse if (ident->tz_begin && ident->tz_end) {\n> +\t\terrno = 0;\n> +\t\ttz = strtol(ident->tz_begin, NULL, 10);\n> +\t\tif (errno)\n\nErrno will trigger if we overflowed a \"long\", but then we assign the\nresult into an int, possibly truncating the result.\n\n> Alternately, the result of strtol() could be assigned temporarily to a\n> 'long', compared against LONG_MIN and LONG_MAX, and then assigned to the\n> 'int' \"tz\" variable.\n\nThat catches overflow from strtol, but we'd then truncate when we pass\nit as an int to show_date.\n\nI think we want this instead:\n\n-- >8 --\nSubject: show_ident_date: fix tz range check\n\nCommit 1dca155fe3fa (log: handle integer overflow in\ntimestamps, 2014-02-24) tried to catch integer overflow\ncoming from strtol() on the timezone field by comparing against\nLONG_MIN/LONG_MAX. However, the intermediate \"tz\" variable\nis an \"int\", which means it can never be LONG_MAX on LP64\nsystems; we would truncate the output from strtol before the\ncomparison.\n\nClang's -Wtautological-constant-out-of-range-compare notices\nthis and rightly complains.\n\nLet's instead store the result of strtol in a long, and then\ncompare it against INT_MIN/INT_MAX. This will catch overflow\nfrom strtol, and also overflow when we pass the result as an\nint to show_date.\n\nReported-by: Eric Sunshine \nSigned-off-by: Jeff King \n---\n pretty.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3b811ed..6e266dd 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -397,7 +397,7 @@ static const char *show_ident_date(const struct ident_split *ident,\n \t\t\t\t enum date_mode mode)\n {\n \tunsigned long date = 0;\n-\tint tz = 0;\n+\tlong tz = 0;\n \n \tif (ident->date_begin && ident->date_end)\n \t\tdate = strtoul(ident->date_begin, NULL, 10);\n@@ -406,7 +406,7 @@ static const char *show_ident_date(const struct ident_split *ident,\n \telse {\n \t\tif (ident->tz_begin && ident->tz_end)\n \t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n-\t\tif (tz == LONG_MAX || tz == LONG_MIN)\n+\t\tif (tz >= INT_MAX || tz <= INT_MIN)\n \t\t\ttz = 0;\n \t}\n \treturn show_date(date, tz, mode);\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236266","messageId":"CAPig+cSgW_B9YEYA3GCD=0NRYS=+QG_v-dU8zp5Jt6XqAysCKA@mail.gmail.com","threadId":"36081","inReplyTo":"20140307171501.GA23587@sigill.intra.peff.net","subject":"Re: [PATCH] show_ident_date: fix always-false conditional","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-07T18:12:58Z","receivedAt":"2014-03-07T18:12:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 7, 2014 at 12:15 PM, Jeff King wrote:\n> On Thu, Mar 06, 2014 at 08:35:24PM -0500, Eric Sunshine wrote:\n>\n>> 1dca155fe3fa (log: handle integer overflow in timestamps, 2014-02-24)\n>> assigns the result of strtol() to an 'int' and then checks it against\n>> LONG_MIN and LONG_MAX, indicating underflow or overflow, even though\n>> 'int' may not be large enough to represent those values.\n>>\n>> On Mac, the compiler complains:\n>>\n>> warning: comparison of constant 9223372036854775807 with\n>> expression of type 'int' is always false\n>> [-Wtautological-constant-out-of-range-compare]\n>> if (<> || tz == LONG_MIN)\n>>\n>> Similarly for the LONG_MIN case. Fix this.\n>\n> Yeah, this is definitely a potential bug. When I added the overflow\n> check, I blindly assumed that the existing code was at least using a\n> sufficiently large type to store the result of strtol, but it's not.\n>\n> I don't think your fix catches all overflow, though:\n>\n>> + else if (ident->tz_begin && ident->tz_end) {\n>> + errno = 0;\n>> + tz = strtol(ident->tz_begin, NULL, 10);\n>> + if (errno)\n>\n> Errno will trigger if we overflowed a \"long\", but then we assign the\n> result into an int, possibly truncating the result.\n>\n>> Alternately, the result of strtol() could be assigned temporarily to a\n>> 'long', compared against LONG_MIN and LONG_MAX, and then assigned to the\n>> 'int' \"tz\" variable.\n>\n> That catches overflow from strtol, but we'd then truncate when we pass\n> it as an int to show_date.\n>\n> I think we want this instead:\n\nMakes sense.\n\nAcked-by: Eric Sunshine \n\n> -- >8 --\n> Subject: show_ident_date: fix tz range check\n>\n> Commit 1dca155fe3fa (log: handle integer overflow in\n> timestamps, 2014-02-24) tried to catch integer overflow\n> coming from strtol() on the timezone field by comparing against\n> LONG_MIN/LONG_MAX. However, the intermediate \"tz\" variable\n> is an \"int\", which means it can never be LONG_MAX on LP64\n> systems; we would truncate the output from strtol before the\n> comparison.\n>\n> Clang's -Wtautological-constant-out-of-range-compare notices\n> this and rightly complains.\n>\n> Let's instead store the result of strtol in a long, and then\n> compare it against INT_MIN/INT_MAX. This will catch overflow\n> from strtol, and also overflow when we pass the result as an\n> int to show_date.\n>\n> Reported-by: Eric Sunshine \n> Signed-off-by: Jeff King \n> ---\n> pretty.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/pretty.c b/pretty.c\n> index 3b811ed..6e266dd 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -397,7 +397,7 @@ static const char *show_ident_date(const struct ident_split *ident,\n> enum date_mode mode)\n> {\n> unsigned long date = 0;\n> - int tz = 0;\n> + long tz = 0;\n>\n> if (ident->date_begin && ident->date_end)\n> date = strtoul(ident->date_begin, NULL, 10);\n> @@ -406,7 +406,7 @@ static const char *show_ident_date(const struct ident_split *ident,\n> else {\n> if (ident->tz_begin && ident->tz_end)\n> tz = strtol(ident->tz_begin, NULL, 10);\n> - if (tz == LONG_MAX || tz == LONG_MIN)\n> + if (tz >= INT_MAX || tz <= INT_MIN)\n> tz = 0;\n> }\n> return show_date(date, tz, mode);\n> --\n> 1.8.5.2.500.g8060133\n>\n"}]}