threads / patch / 25349

v3do not depend on signed integer overflow

Subject: [PATCH v3] do not depend on signed integer overflow

## tl;dr

4 messages between Oct 4, 2010 and Oct 5, 2010. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Erik Faye-Lund· Oct 4, 2010, 22:49 UTC · lore
Signed integer overflow is not defined in C, so do not depend on it.

This fixes a problem with GCC 4.4.0 and -O3 where the optimizer would consider "consumed_bytes > consumed_bytes + bytes" as a constant expression, and never execute the die()-call.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 builtin/index-pack.c     |    2 +-
 builtin/unpack-objects.c |    2 +-
 git-compat-util.h        |   12 ++++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)
Show changes to 3 files +14 −2

builtin/index-pack.c, builtin/unpack-objects.c, git-compat-util.h

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2e680d7..e243d9d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -161,7 +161,7 @@ static void use(int bytes)
 	input_offset += bytes;
 
 	/* make sure off_t is sufficiently large not to wrap */
-	if (consumed_bytes > consumed_bytes + bytes)
+	if (signed_add_overflows(consumed_bytes, bytes))
 		die("pack too large for current definition of off_t");
 	consumed_bytes += bytes;
 }
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 685566e..f63973c 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -83,7 +83,7 @@ static void use(int bytes)
 	offset += bytes;
 
 	/* make sure off_t is sufficiently large not to wrap */
-	if (consumed_bytes > consumed_bytes + bytes)
+	if (signed_add_overflows(consumed_bytes, bytes))
 		die("pack too large for current definition of off_t");
 	consumed_bytes += bytes;
 }
diff --git a/git-compat-util.h b/git-compat-util.h
index 81883e7..2af8d3e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -28,6 +28,18 @@
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 #define bitsizeof(x)  (CHAR_BIT * sizeof(x))
 
+#define maximum_signed_value_of_type(a) \
+    (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))
+
+/*
+ * Signed integer overflow is undefined in C, so here's a helper macro
+ * to detect if the sum of two integers will overflow.
+ *
+ * Requires: a >= 0, typeof(a) equals typeof(b)
+ */
+#define signed_add_overflows(a, b) \
+    ((b) > maximum_signed_value_of_type(a) - (a))
+
 #ifdef __GNUC__
 #define TYPEOF(x) (__typeof__(x))
 #else
-- 
1.7.3.1.51.g3f36d.dirty
Jeff King· Oct 5, 2010, 03:43 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v3] do not depend on signed integer overflow

On Tue, Oct 05, 2010 at 12:49:12AM +0200, Erik Faye-Lund wrote:
Show 13 quoted lines
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 2e680d7..e243d9d 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -161,7 +161,7 @@ static void use(int bytes)
>  	input_offset += bytes;
>  
>  	/* make sure off_t is sufficiently large not to wrap */
> -	if (consumed_bytes > consumed_bytes + bytes)
> +	if (signed_add_overflows(consumed_bytes, bytes))
>  		die("pack too large for current definition of off_t");
>  	consumed_bytes += bytes;
>  }

FWIW, I find your new version much easier to read, too (that it fixes a bug is just a nice bonus. ;) ).

-Peff
Nicolas Pitre· Oct 5, 2010, 04:17 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v3] do not depend on signed integer overflow

On Tue, 5 Oct 2010, Erik Faye-Lund wrote:
Show 7 quoted lines
> Signed integer overflow is not defined in C, so do not depend on it.
> 
> This fixes a problem with GCC 4.4.0 and -O3 where the optimizer would
> consider "consumed_bytes > consumed_bytes + bytes" as a constant
> expression, and never execute the die()-call.
> 
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>

I like this. Please also fix the similar case in builtin/pack-objects.c:write_one().

Show 63 quoted lines
> ---
>  builtin/index-pack.c     |    2 +-
>  builtin/unpack-objects.c |    2 +-
>  git-compat-util.h        |   12 ++++++++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 2e680d7..e243d9d 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -161,7 +161,7 @@ static void use(int bytes)
>  	input_offset += bytes;
>  
>  	/* make sure off_t is sufficiently large not to wrap */
> -	if (consumed_bytes > consumed_bytes + bytes)
> +	if (signed_add_overflows(consumed_bytes, bytes))
>  		die("pack too large for current definition of off_t");
>  	consumed_bytes += bytes;
>  }
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 685566e..f63973c 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -83,7 +83,7 @@ static void use(int bytes)
>  	offset += bytes;
>  
>  	/* make sure off_t is sufficiently large not to wrap */
> -	if (consumed_bytes > consumed_bytes + bytes)
> +	if (signed_add_overflows(consumed_bytes, bytes))
>  		die("pack too large for current definition of off_t");
>  	consumed_bytes += bytes;
>  }
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 81883e7..2af8d3e 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -28,6 +28,18 @@
>  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
>  #define bitsizeof(x)  (CHAR_BIT * sizeof(x))
>  
> +#define maximum_signed_value_of_type(a) \
> +    (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))
> +
> +/*
> + * Signed integer overflow is undefined in C, so here's a helper macro
> + * to detect if the sum of two integers will overflow.
> + *
> + * Requires: a >= 0, typeof(a) equals typeof(b)
> + */
> +#define signed_add_overflows(a, b) \
> +    ((b) > maximum_signed_value_of_type(a) - (a))
> +
>  #ifdef __GNUC__
>  #define TYPEOF(x) (__typeof__(x))
>  #else
> -- 
> 1.7.3.1.51.g3f36d.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Erik Faye-Lund· Oct 5, 2010, 07:26 UTC · re: Nicolas Pitre · lore

Re: [PATCH v3] do not depend on signed integer overflow

On Tue, Oct 5, 2010 at 6:17 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
Show 13 quoted lines
> On Tue, 5 Oct 2010, Erik Faye-Lund wrote:
>
>> Signed integer overflow is not defined in C, so do not depend on it.
>>
>> This fixes a problem with GCC 4.4.0 and -O3 where the optimizer would
>> consider "consumed_bytes > consumed_bytes + bytes" as a constant
>> expression, and never execute the die()-call.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>
> I like this.  Please also fix the similar case in
> builtin/pack-objects.c:write_one().
>

Thanks for pointing that one out. For some reason GCC didn't warn about that instance - perhaps it's optimizer missed the case due to going through that pointer?

I've just resent a version with that fix-up.

← back to recent threads