{"thread":{"id":"25349","subject":"[PATCH v3] do not depend on signed integer overflow","startedAt":"2010-10-04T22:49:12Z","lastAt":"2010-10-05T07:26:26Z","messageCount":4,"participants":["Erik Faye-Lund","Jeff King","Nicolas Pitre"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"152615","messageId":"1286232552-5244-1-git-send-email-kusmabite@gmail.com","threadId":"25349","inReplyTo":null,"subject":"[PATCH v3] do not depend on signed integer overflow","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-10-04T22:49:12Z","receivedAt":"2010-10-04T22:49:12Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Signed integer overflow is not defined in C, so do not depend on it.\n\nThis fixes a problem with GCC 4.4.0 and -O3 where the optimizer would\nconsider \"consumed_bytes > consumed_bytes + bytes\" as a constant\nexpression, and never execute the die()-call.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n builtin/index-pack.c     |    2 +-\n builtin/unpack-objects.c |    2 +-\n git-compat-util.h        |   12 ++++++++++++\n 3 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 2e680d7..e243d9d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -161,7 +161,7 @@ static void use(int bytes)\n \tinput_offset += bytes;\n \n \t/* make sure off_t is sufficiently large not to wrap */\n-\tif (consumed_bytes > consumed_bytes + bytes)\n+\tif (signed_add_overflows(consumed_bytes, bytes))\n \t\tdie(\"pack too large for current definition of off_t\");\n \tconsumed_bytes += bytes;\n }\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 685566e..f63973c 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -83,7 +83,7 @@ static void use(int bytes)\n \toffset += bytes;\n \n \t/* make sure off_t is sufficiently large not to wrap */\n-\tif (consumed_bytes > consumed_bytes + bytes)\n+\tif (signed_add_overflows(consumed_bytes, bytes))\n \t\tdie(\"pack too large for current definition of off_t\");\n \tconsumed_bytes += bytes;\n }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 81883e7..2af8d3e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -28,6 +28,18 @@\n #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))\n #define bitsizeof(x)  (CHAR_BIT * sizeof(x))\n \n+#define maximum_signed_value_of_type(a) \\\n+    (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))\n+\n+/*\n+ * Signed integer overflow is undefined in C, so here's a helper macro\n+ * to detect if the sum of two integers will overflow.\n+ *\n+ * Requires: a >= 0, typeof(a) equals typeof(b)\n+ */\n+#define signed_add_overflows(a, b) \\\n+    ((b) > maximum_signed_value_of_type(a) - (a))\n+\n #ifdef __GNUC__\n #define TYPEOF(x) (__typeof__(x))\n #else\n-- \n1.7.3.1.51.g3f36d.dirty\n"},{"id":"152640","messageId":"20101005034301.GA23357@coredump.intra.peff.net","threadId":"25349","inReplyTo":"1286232552-5244-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v3] do not depend on signed integer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-10-05T03:43:01Z","receivedAt":"2010-10-05T03:43:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 05, 2010 at 12:49:12AM +0200, Erik Faye-Lund wrote:\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 2e680d7..e243d9d 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -161,7 +161,7 @@ static void use(int bytes)\n>  \tinput_offset += bytes;\n>  \n>  \t/* make sure off_t is sufficiently large not to wrap */\n> -\tif (consumed_bytes > consumed_bytes + bytes)\n> +\tif (signed_add_overflows(consumed_bytes, bytes))\n>  \t\tdie(\"pack too large for current definition of off_t\");\n>  \tconsumed_bytes += bytes;\n>  }\n\nFWIW, I find your new version much easier to read, too (that it fixes a\nbug is just a nice bonus. ;) ).\n\n-Peff\n"},{"id":"152641","messageId":"alpine.LFD.2.00.1010050012490.3107@xanadu.home","threadId":"25349","inReplyTo":"1286232552-5244-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v3] do not depend on signed integer overflow","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-10-05T04:17:12Z","receivedAt":"2010-10-05T04:17:12Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 5 Oct 2010, Erik Faye-Lund wrote:\n\n> Signed integer overflow is not defined in C, so do not depend on it.\n> \n> This fixes a problem with GCC 4.4.0 and -O3 where the optimizer would\n> consider \"consumed_bytes > consumed_bytes + bytes\" as a constant\n> expression, and never execute the die()-call.\n> \n> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n\nI like this.  Please also fix the similar case in \nbuiltin/pack-objects.c:write_one().\n\n> ---\n>  builtin/index-pack.c     |    2 +-\n>  builtin/unpack-objects.c |    2 +-\n>  git-compat-util.h        |   12 ++++++++++++\n>  3 files changed, 14 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 2e680d7..e243d9d 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -161,7 +161,7 @@ static void use(int bytes)\n>  \tinput_offset += bytes;\n>  \n>  \t/* make sure off_t is sufficiently large not to wrap */\n> -\tif (consumed_bytes > consumed_bytes + bytes)\n> +\tif (signed_add_overflows(consumed_bytes, bytes))\n>  \t\tdie(\"pack too large for current definition of off_t\");\n>  \tconsumed_bytes += bytes;\n>  }\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index 685566e..f63973c 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -83,7 +83,7 @@ static void use(int bytes)\n>  \toffset += bytes;\n>  \n>  \t/* make sure off_t is sufficiently large not to wrap */\n> -\tif (consumed_bytes > consumed_bytes + bytes)\n> +\tif (signed_add_overflows(consumed_bytes, bytes))\n>  \t\tdie(\"pack too large for current definition of off_t\");\n>  \tconsumed_bytes += bytes;\n>  }\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 81883e7..2af8d3e 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -28,6 +28,18 @@\n>  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))\n>  #define bitsizeof(x)  (CHAR_BIT * sizeof(x))\n>  \n> +#define maximum_signed_value_of_type(a) \\\n> +    (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))\n> +\n> +/*\n> + * Signed integer overflow is undefined in C, so here's a helper macro\n> + * to detect if the sum of two integers will overflow.\n> + *\n> + * Requires: a >= 0, typeof(a) equals typeof(b)\n> + */\n> +#define signed_add_overflows(a, b) \\\n> +    ((b) > maximum_signed_value_of_type(a) - (a))\n> +\n>  #ifdef __GNUC__\n>  #define TYPEOF(x) (__typeof__(x))\n>  #else\n> -- \n> 1.7.3.1.51.g3f36d.dirty\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"152648","messageId":"AANLkTimdaghq5_vcb+7Anh_05oLJUbZuNXU47YWdBudq@mail.gmail.com","threadId":"25349","inReplyTo":"alpine.LFD.2.00.1010050012490.3107@xanadu.home","subject":"Re: [PATCH v3] do not depend on signed integer overflow","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-10-05T07:26:26Z","receivedAt":"2010-10-05T07:26:26Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Oct 5, 2010 at 6:17 AM, Nicolas Pitre <nico@fluxnic.net> wrote:\n> On Tue, 5 Oct 2010, Erik Faye-Lund wrote:\n>\n>> Signed integer overflow is not defined in C, so do not depend on it.\n>>\n>> This fixes a problem with GCC 4.4.0 and -O3 where the optimizer would\n>> consider \"consumed_bytes > consumed_bytes + bytes\" as a constant\n>> expression, and never execute the die()-call.\n>>\n>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n>\n> I like this.  Please also fix the similar case in\n> builtin/pack-objects.c:write_one().\n>\n\nThanks for pointing that one out. For some reason GCC didn't warn\nabout that instance - perhaps it's optimizer missed the case due to\ngoing through that pointer?\n\nI've just resent a version with that fix-up.\n"}]}