From 2884cc42de8b20a58cec8488d014a853d47c047e Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Wed, 11 Feb 2015 09:39:38 -0500 Subject: overflow checking: don't make callers set oom Have the ALLOC_OVERFLOW testing macros also simply set_oom in the case where a computation would overflow, so that callers don't need to. --- src/buffer.c | 1 - src/common.h | 14 ++++---------- src/config_file.c | 2 -- src/delta.c | 2 +- src/index.c | 4 +--- src/pack-objects.c | 4 +--- src/pool.c | 4 +--- src/refs.c | 4 +--- src/tree.c | 4 +--- src/util.h | 15 ++++----------- src/win32/dir.c | 4 +--- tests/core/errors.c | 21 +++++++++++++++++++++ 12 files changed, 36 insertions(+), 43 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 8098bb1e7..0d0314439 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -105,7 +105,6 @@ int git_buf_grow_by(git_buf *buffer, size_t additional_size) { if (GIT_ALLOC_OVERFLOW_ADD(buffer->size, additional_size)) { buffer->ptr = git_buf__oom; - giterr_set_oom(); return -1; } diff --git a/src/common.h b/src/common.h index b53798eaf..8b7d6936d 100644 --- a/src/common.h +++ b/src/common.h @@ -176,25 +176,19 @@ GIT_INLINE(void) git__init_structure(void *structure, size_t len, unsigned int v /** Check for integer overflow from addition or multiplication */ #define GIT_ALLOC_OVERFLOW_ADD(one, two) \ - ((one) + (two) < (one)) + (((one) + (two) < (one)) ? (giterr_set_oom(), 1) : 0) /** Check for integer overflow from multiplication */ #define GIT_ALLOC_OVERFLOW_MULTIPLY(one, two) \ - (one && ((one) * (two)) / (one) != (two)) + ((one && ((one) * (two)) / (one) != (two)) ? (giterr_set_oom(), 1) : 0) /** Check for additive overflow, failing if it would occur. */ #define GITERR_CHECK_ALLOC_ADD(one, two) \ - if (GIT_ALLOC_OVERFLOW_ADD(one, two)) { \ - giterr_set_oom(); \ - return -1; \ - } + if (GIT_ALLOC_OVERFLOW_ADD(one, two)) { return -1; } /** Check for multiplicative overflow, failing if it would occur. */ #define GITERR_CHECK_ALLOC_MULTIPLY(nelem, elsize) \ - if (GIT_ALLOC_OVERFLOW_MULTIPLY(nelem, elsize)) { \ - giterr_set_oom(); \ - return -1; \ - } + if (GIT_ALLOC_OVERFLOW_MULTIPLY(nelem, elsize)) { return -1; } /* NOTE: other giterr functions are in the public errors.h header file */ diff --git a/src/config_file.c b/src/config_file.c index 36f78563b..39e9ff841 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -905,7 +905,6 @@ static char *reader_readline(struct reader *reader, bool skip_whitespace) if (GIT_ALLOC_OVERFLOW_ADD(line_len, 1) || (line = git__malloc(line_len + 1)) == NULL) { - giterr_set_oom(); return NULL; } @@ -1620,7 +1619,6 @@ static char *fixup_line(const char *ptr, int quote_count) if (GIT_ALLOC_OVERFLOW_ADD(ptr_len, 1) || (str = git__malloc(ptr_len + 1)) == NULL) { - giterr_set_oom(); return NULL; } diff --git a/src/delta.c b/src/delta.c index f704fdfb1..242f3abe3 100644 --- a/src/delta.c +++ b/src/delta.c @@ -138,7 +138,7 @@ static int lookup_index_alloc( index_len += hash_len; if (!git__is_ulong(index_len)) { - giterr_set_oom(); + giterr_set(GITERR_NOMEMORY, "Overly large delta"); return -1; } diff --git a/src/index.c b/src/index.c index 70090d12c..35f512ca4 100644 --- a/src/index.c +++ b/src/index.c @@ -833,10 +833,8 @@ static git_index_reuc_entry *reuc_entry_alloc(const char *path) struct reuc_entry_internal *entry; if (GIT_ALLOC_OVERFLOW_ADD(structlen, pathlen) || - GIT_ALLOC_OVERFLOW_ADD(structlen + pathlen, 1)) { - giterr_set_oom(); + GIT_ALLOC_OVERFLOW_ADD(structlen + pathlen, 1)) return NULL; - } entry = git__calloc(1, structlen + pathlen + 1); if (!entry) diff --git a/src/pack-objects.c b/src/pack-objects.c index cd0deb29a..288077078 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -504,10 +504,8 @@ static git_pobject **compute_write_order(git_packbuilder *pb) unsigned int i, wo_end, last_untagged; git_pobject **wo; - if ((wo = git__mallocarray(pb->nr_objects, sizeof(*wo))) == NULL) { - giterr_set_oom(); + if ((wo = git__mallocarray(pb->nr_objects, sizeof(*wo))) == NULL) return NULL; - } for (i = 0; i < pb->nr_objects; i++) { git_pobject *po = pb->object_list + i; diff --git a/src/pool.c b/src/pool.c index 1599fc7a2..919e13d08 100644 --- a/src/pool.c +++ b/src/pool.c @@ -117,10 +117,8 @@ static void *pool_alloc_page(git_pool *pool, uint32_t size) } if (GIT_ALLOC_OVERFLOW_ADD(alloc_size, sizeof(git_pool_page)) || - !(page = git__calloc(1, alloc_size + sizeof(git_pool_page)))) { - giterr_set_oom(); + !(page = git__calloc(1, alloc_size + sizeof(git_pool_page)))) return NULL; - } page->size = alloc_size; page->avail = alloc_size - size; diff --git a/src/refs.c b/src/refs.c index 33e931db5..af3190ef9 100644 --- a/src/refs.c +++ b/src/refs.c @@ -41,10 +41,8 @@ static git_reference *alloc_ref(const char *name) if (GIT_ALLOC_OVERFLOW_ADD(reflen, namelen) || GIT_ALLOC_OVERFLOW_ADD(reflen + namelen, 1) || - (ref = git__calloc(1, reflen + namelen + 1)) == NULL) { - giterr_set_oom(); + (ref = git__calloc(1, reflen + namelen + 1)) == NULL) return NULL; - } memcpy(ref->name, name, namelen + 1); diff --git a/src/tree.c b/src/tree.c index 2c8b89291..573e56447 100644 --- a/src/tree.c +++ b/src/tree.c @@ -89,10 +89,8 @@ static git_tree_entry *alloc_entry(const char *filename) if (GIT_ALLOC_OVERFLOW_ADD(tree_len, filename_len) || GIT_ALLOC_OVERFLOW_ADD(tree_len + filename_len, 1) || - !(entry = git__malloc(tree_len + filename_len + 1))) { - giterr_set_oom(); + !(entry = git__malloc(tree_len + filename_len + 1))) return NULL; - } memset(entry, 0x0, sizeof(git_tree_entry)); memcpy(entry->filename, filename, filename_len); diff --git a/src/util.h b/src/util.h index 7693ef809..40e06976d 100644 --- a/src/util.h +++ b/src/util.h @@ -64,10 +64,8 @@ GIT_INLINE(char *) git__strndup(const char *str, size_t n) length = p_strnlen(str, n); - if (GIT_ALLOC_OVERFLOW_ADD(length, 1)) { - giterr_set_oom(); + if (GIT_ALLOC_OVERFLOW_ADD(length, 1)) return NULL; - } ptr = git__malloc(length + 1); @@ -87,10 +85,8 @@ GIT_INLINE(char *) git__substrdup(const char *start, size_t n) { char *ptr; - if (GIT_ALLOC_OVERFLOW_ADD(n, 1) || !(ptr = git__malloc(n+1))) { - giterr_set_oom(); + if (GIT_ALLOC_OVERFLOW_ADD(n, 1) || !(ptr = git__malloc(n+1))) return NULL; - } memcpy(ptr, start, n); ptr[n] = '\0'; @@ -111,11 +107,8 @@ GIT_INLINE(void *) git__realloc(void *ptr, size_t size) */ GIT_INLINE(void *) git__reallocarray(void *ptr, size_t nelem, size_t elsize) { - void *new_ptr = NULL; - if (GIT_ALLOC_OVERFLOW_MULTIPLY(nelem, elsize) || - !(new_ptr = realloc(ptr, nelem * elsize))) - giterr_set_oom(); - return new_ptr; + return GIT_ALLOC_OVERFLOW_MULTIPLY(nelem, elsize) ? + NULL : realloc(ptr, nelem * elsize); } /** diff --git a/src/win32/dir.c b/src/win32/dir.c index 9953289f6..7f2a5a56e 100644 --- a/src/win32/dir.c +++ b/src/win32/dir.c @@ -20,10 +20,8 @@ git__DIR *git__opendir(const char *dir) if (GIT_ALLOC_OVERFLOW_ADD(sizeof(*new), dirlen) || GIT_ALLOC_OVERFLOW_ADD(sizeof(*new) + dirlen, 1) || - !(new = git__calloc(1, sizeof(*new) + dirlen + 1))) { - giterr_set_oom(); + !(new = git__calloc(1, sizeof(*new) + dirlen + 1))) return NULL; - } memcpy(new->dir, dir, dirlen); diff --git a/tests/core/errors.c b/tests/core/errors.c index b6364100b..11e0bb2bb 100644 --- a/tests/core/errors.c +++ b/tests/core/errors.c @@ -149,3 +149,24 @@ void test_core_errors__integer_overflow_alloc_add(void) cl_assert_equal_i(GITERR_NOMEMORY, giterr_last()->klass); cl_assert_equal_s("Out of memory", giterr_last()->message); } + +void test_core_errors__integer_overflow_sets_oom(void) +{ + giterr_clear(); + cl_assert(!GIT_ALLOC_OVERFLOW_ADD(SIZE_MAX-1, 1)); + cl_assert_equal_p(NULL, giterr_last()); + + giterr_clear(); + cl_assert(!GIT_ALLOC_OVERFLOW_ADD(42, 69)); + cl_assert_equal_p(NULL, giterr_last()); + + giterr_clear(); + cl_assert(GIT_ALLOC_OVERFLOW_ADD(SIZE_MAX, SIZE_MAX)); + cl_assert_equal_i(GITERR_NOMEMORY, giterr_last()->klass); + cl_assert_equal_s("Out of memory", giterr_last()->message); + + giterr_clear(); + cl_assert(GIT_ALLOC_OVERFLOW_MULTIPLY(SIZE_MAX, SIZE_MAX)); + cl_assert_equal_i(GITERR_NOMEMORY, giterr_last()->klass); + cl_assert_equal_s("Out of memory", giterr_last()->message); +} -- cgit v1.2.1