# [PATCH v4] do not depend on signed integer overflow 5 messages from 2010-10-05 to 2011-02-10. Participants: Erik Faye-Lund, Nicolas Pitre, Jonathan Nieder, Sverre Rabbelier, Joshua Juran. Thread: https://gitlist.dev/t/25352 ## Erik Faye-Lund, 2010-10-05 07:24 Subject: [PATCH v4] do not depend on signed integer overflow Message-ID: <1286263450-5372-1-git-send-email-kusmabite@gmail.com> URL: https://gitlist.dev/e/1286263450-5372-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/pack-objects.c | 2 +- builtin/unpack-objects.c | 2 +- git-compat-util.h | 12 ++++++++++++ 4 files changed, 15 insertions(+), 3 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/pack-objects.c b/builtin/pack-objects.c index 3756cf3..d5a8db1 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -431,7 +431,7 @@ static int write_one(struct sha1file *f, written_list[nr_written++] = &e->idx; /* make sure off_t is sufficiently large not to wrap */ - if (*offset > *offset + size) + if (signed_add_overflows(*offset, size)) die("pack too large for current definition of off_t"); *offset += size; return 1; 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.ge462f.dirty ``` ## Nicolas Pitre, 2010-10-05 13:28 Subject: Re: [PATCH v4] do not depend on signed integer overflow Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1010050928200.3107%40xanadu.home In-Reply-To: <1286263450-5372-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 Acked-by: Nicolas Pitre > --- > builtin/index-pack.c | 2 +- > builtin/pack-objects.c | 2 +- > builtin/unpack-objects.c | 2 +- > git-compat-util.h | 12 ++++++++++++ > 4 files changed, 15 insertions(+), 3 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/pack-objects.c b/builtin/pack-objects.c > index 3756cf3..d5a8db1 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -431,7 +431,7 @@ static int write_one(struct sha1file *f, > written_list[nr_written++] = &e->idx; > > /* make sure off_t is sufficiently large not to wrap */ > - if (*offset > *offset + size) > + if (signed_add_overflows(*offset, size)) > die("pack too large for current definition of off_t"); > *offset += size; > return 1; > 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.ge462f.dirty > ``` ## Jonathan Nieder, 2011-02-10 09:35 Subject: [PATCH] compat: helper for detecting unsigned overflow Message-ID: <20110210093536.GB365@elie> URL: https://gitlist.dev/e/20110210093536.GB365%40elie In-Reply-To: <1286263450-5372-1-git-send-email-kusmabite@gmail.com> ``` Date: Sun, 10 Oct 2010 21:59:26 -0500 The idiom (a + b < a) works fine for detecting that an unsigned integer has overflowed, but a more explicit unsigned_add_overflows(a, b) might be easier to read. Define such a macro, expanding roughly to ((a) < UINT_MAX - (b)). Because the expansion uses each argument only once outside of sizeof() expressions, it is safe to use with arguments that have side effects. Signed-off-by: Jonathan Nieder --- From the svn remote helper project. Sane? git-compat-util.h | 6 ++++++ patch-delta.c | 2 +- strbuf.c | 5 +++-- wrapper.c | 2 +- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index d6d269f..9c23622 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -31,6 +31,9 @@ #define maximum_signed_value_of_type(a) \ (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a))) +#define maximum_unsigned_value_of_type(a) \ + (UINTMAX_MAX >> (bitsizeof(uintmax_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. @@ -40,6 +43,9 @@ #define signed_add_overflows(a, b) \ ((b) > maximum_signed_value_of_type(a) - (a)) +#define unsigned_add_overflows(a, b) \ + ((b) > maximum_unsigned_value_of_type(a) - (a)) + #ifdef __GNUC__ #define TYPEOF(x) (__typeof__(x)) #else diff --git a/patch-delta.c b/patch-delta.c index d218faa..56e0a5e 100644 --- a/patch-delta.c +++ b/patch-delta.c @@ -48,7 +48,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size, if (cmd & 0x20) cp_size |= (*data++ << 8); if (cmd & 0x40) cp_size |= (*data++ << 16); if (cp_size == 0) cp_size = 0x10000; - if (cp_off + cp_size < cp_size || + if (unsigned_add_overflows(cp_off, cp_size) || cp_off + cp_size > src_size || cp_size > size) break; diff --git a/strbuf.c b/strbuf.c index 9b3c445..07e8883 100644 --- a/strbuf.c +++ b/strbuf.c @@ -63,7 +63,8 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc) void strbuf_grow(struct strbuf *sb, size_t extra) { - if (sb->len + extra + 1 <= sb->len) + if (unsigned_add_overflows(extra, 1) || + unsigned_add_overflows(sb->len, extra + 1)) die("you want to use way too much memory"); if (!sb->alloc) sb->buf = NULL; @@ -152,7 +153,7 @@ int strbuf_cmp(const struct strbuf *a, const struct strbuf *b) void strbuf_splice(struct strbuf *sb, size_t pos, size_t len, const void *data, size_t dlen) { - if (pos + len < pos) + if (unsigned_add_overflows(pos, len)) die("you want to use way too much memory"); if (pos > sb->len) die("`pos' is too far after the end of the buffer"); diff --git a/wrapper.c b/wrapper.c index 55b074e..4c147d6 100644 --- a/wrapper.c +++ b/wrapper.c @@ -53,7 +53,7 @@ void *xmalloc(size_t size) void *xmallocz(size_t size) { void *ret; - if (size + 1 < size) + if (unsigned_add_overflows(size, 1)) die("Data too large to fit into virtual memory space."); ret = xmalloc(size + 1); ((char*)ret)[size] = 0; -- 1.7.4 ``` ## Sverre Rabbelier, 2011-02-10 12:11 Subject: Re: [PATCH] compat: helper for detecting unsigned overflow Message-ID: URL: https://gitlist.dev/e/AANLkTinqkSxj0C%2BGyMVR4a7d%3Dyy_mh%2BoDuar0moZjZ8_%40mail.gmail.com In-Reply-To: <20110210093536.GB365@elie> ``` Heya, On Thu, Feb 10, 2011 at 10:35, Jonathan Nieder wrote: > unsigned_add_overflows(a, b) > Define such a macro, expanding roughly to ((a) < UINT_MAX - (b)). > Because the expansion uses each argument only once outside of sizeof() > expressions, it is safe to use with arguments that have side effects. > +#define unsigned_add_overflows(a, b) \ > +    ((b) > maximum_unsigned_value_of_type(a) - (a)) I'm confused, you say you won't use it twice, and then you do use it twice? -- Cheers, Sverre Rabbelier ``` ## Joshua Juran, 2011-02-10 13:23 Subject: Re: [PATCH] compat: helper for detecting unsigned overflow Message-ID: URL: https://gitlist.dev/e/E84FF9D8-92D7-4CA0-9CF4-77FF33893977%40gmail.com In-Reply-To: ``` On Feb 10, 2011, at 4:11 AM, Sverre Rabbelier wrote: > On Thu, Feb 10, 2011 at 10:35, Jonathan Nieder > wrote: >> unsigned_add_overflows(a, b) > >> Define such a macro, expanding roughly to ((a) < UINT_MAX - (b)). >> Because the expansion uses each argument only once outside of >> sizeof() >> expressions, it is safe to use with arguments that have side effects. > >> +#define unsigned_add_overflows(a, b) \ >> + ((b) > maximum_unsigned_value_of_type(a) - (a)) > > I'm confused, you say you won't use it twice, and then you do use it > twice? The author is asserting that maximum_unsigned_value_of_type() is a function of sizeof a, which would have no runtime effect. Josh ```