# [PATCH v3] do not depend on signed integer overflow 4 messages from 2010-10-04 to 2010-10-05. Participants: Erik Faye-Lund, Jeff King, Nicolas Pitre. Thread: https://gitlist.dev/t/25349 ## Erik Faye-Lund, 2010-10-04 22:49 Subject: [PATCH v3] do not depend on signed integer overflow Message-ID: <1286232552-5244-1-git-send-email-kusmabite@gmail.com> URL: https://gitlist.dev/e/1286232552-5244-1-git-send-email-kusmabite%40gmail.com ``` 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 --- 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 ``` ## Jeff King, 2010-10-05 03:43 Subject: Re: [PATCH v3] do not depend on signed integer overflow Message-ID: <20101005034301.GA23357@coredump.intra.peff.net> URL: https://gitlist.dev/e/20101005034301.GA23357%40coredump.intra.peff.net In-Reply-To: <1286232552-5244-1-git-send-email-kusmabite@gmail.com> ``` On Tue, Oct 05, 2010 at 12:49:12AM +0200, Erik Faye-Lund wrote: > 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, 2010-10-05 04:17 Subject: Re: [PATCH v3] do not depend on signed integer overflow Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1010050012490.3107%40xanadu.home In-Reply-To: <1286232552-5244-1-git-send-email-kusmabite@gmail.com> ``` 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 I like this. Please also fix the similar case in builtin/pack-objects.c:write_one(). > --- > 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, 2010-10-05 07:26 Subject: Re: [PATCH v3] do not depend on signed integer overflow Message-ID: URL: https://gitlist.dev/e/AANLkTimdaghq5_vcb%2B7Anh_05oLJUbZuNXU47YWdBudq%40mail.gmail.com In-Reply-To: ``` On Tue, Oct 5, 2010 at 6:17 AM, Nicolas Pitre wrote: > 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 > > 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. ```