[PATCH 1/2] add COPY_ARRAY

[PATCH 1/2] add COPY_ARRAY

[René Scharfe]

Add COPY_ARRAY, a safe and convenient helper for copying arrays,
complementing ALLOC_ARRAY and REALLOC_ARRAY.  Users just specify source,
destination and the number of elements; the size of an element is
inferred automatically.

It checks if the multiplication of size and element count overflows.
The inferred size is passed first to st_mult, which allows the division
there to be done at compilation time.

As a basic type safety check it makes sure the sizes of source and
destination elements are the same.  That's evaluated at compilation time
as well.

COPY_ARRAY is safe to use with NULL as source pointer iff 0 elements are
to be copied.  That convention is used in some cases for initializing
arrays.  Raw memcpy(3) does not support it -- compilers are allowed to
assume that only valid pointers are passed to it and can optimize away
NULL checks after such a call.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 37cce07..91775ce 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -801,6 +801,14 @@ extern FILE *fopen_for_writing(const char *path);
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
 
+#define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
+	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
+static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
+{
+	if (n)
+		memcpy(dst, src, st_mult(size, n));
+}
+
 /*
  * These functions help you allocate structs with flex arrays, and copy
  * the data directly into the array. For example, if you had:
-- 
2.10.0

[PATCH 6/7] use COPY_ARRAY

[René Scharfe]

Add a semantic patch for converting certain calls of memcpy(3) to
COPY_ARRAY() and apply that transformation to the code base.  The result
is
 shorter and safer code.  For now only consider calls where source and
destination have the same type, or in other words: easy cases.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/mv.c                   |  2 +-
 commit.c                       |  2 +-
 contrib/coccinelle/array.cocci | 26 ++++++++++++++++++++++++++
 pack-revindex.c                |  2 +-
 pathspec.c                     |  3 +--
 split-index.c                  |  6 ++----
 6 files changed, 32 insertions(+), 9 deletions(-)
 create mode 100644 contrib/coccinelle/array.cocci

diff --git a/builtin/mv.c b/builtin/mv.c
index 446a316..2f43877 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -26,7 +26,7 @@ static const char **internal_copy_pathspec(const char *prefix,
 	int i;
 	const char **result;
 	ALLOC_ARRAY(result, count + 1);
-	memcpy(result, pathspec, count * sizeof(const char *));
+	COPY_ARRAY(result, pathspec, count);
 	result[count] = NULL;
 	for (i = 0; i < count; i++) {
 		int length = strlen(result[i]);
diff --git a/commit.c b/commit.c
index ba6dee3..aada266 100644
--- a/commit.c
+++ b/commit.c
@@ -931,7 +931,7 @@ static int remove_redundant(struct commit **array, int cnt)
 	}
 
 	/* Now collect the result */
-	memcpy(work, array, sizeof(*array) * cnt);
+	COPY_ARRAY(work, array, cnt);
 	for (i = filled = 0; i < cnt; i++)
 		if (!redundant[i])
 			array[filled++] = work[i];
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
new file mode 100644
index 0000000..2d7f25d
--- /dev/null
+++ b/contrib/coccinelle/array.cocci
@@ -0,0 +1,26 @@
+@@
+type T;
+T *dst;
+T *src;
+expression n;
+@@
+- memcpy(dst, src, n * sizeof(*dst));
++ COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *dst;
+T *src;
+expression n;
+@@
+- memcpy(dst, src, n * sizeof(*src));
++ COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *dst;
+T *src;
+expression n;
+@@
+- memcpy(dst, src, n * sizeof(T));
++ COPY_ARRAY(dst, src, n);
diff --git a/pack-revindex.c b/pack-revindex.c
index 96d51c3..6bc7c94 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -107,7 +107,7 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)
 	 * we have to move it back from the temporary storage.
 	 */
 	if (from != entries)
-		memcpy(entries, tmp, n * sizeof(*entries));
+		COPY_ARRAY(entries, tmp, n);
 	free(tmp);
 	free(pos);
 
diff --git a/pathspec.c b/pathspec.c
index 24e0dd5..49a5360 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -485,8 +485,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 {
 	*dst = *src;
 	ALLOC_ARRAY(dst->items, dst->nr);
-	memcpy(dst->items, src->items,
-	       sizeof(struct pathspec_item) * dst->nr);
+	COPY_ARRAY(dst->items, src->items, dst->nr);
 }
 
 void clear_pathspec(struct pathspec *pathspec)
diff --git a/split-index.c b/split-index.c
index 3c75d4b..35da553 100644
--- a/split-index.c
+++ b/split-index.c
@@ -83,8 +83,7 @@ void move_cache_to_base_index(struct index_state *istate)
 	si->base->timestamp = istate->timestamp;
 	ALLOC_GROW(si->base->cache, istate->cache_nr, si->base->cache_alloc);
 	si->base->cache_nr = istate->cache_nr;
-	memcpy(si->base->cache, istate->cache,
-	       sizeof(*istate->cache) * istate->cache_nr);
+	COPY_ARRAY(si->base->cache, istate->cache, istate->cache_nr);
 	mark_base_index_entries(si->base);
 	for (i = 0; i < si->base->cache_nr; i++)
 		si->base->cache[i]->ce_flags &= ~CE_UPDATE_IN_BASE;
@@ -141,8 +140,7 @@ void merge_base_index(struct index_state *istate)
 	istate->cache	    = NULL;
 	istate->cache_alloc = 0;
 	ALLOC_GROW(istate->cache, istate->cache_nr, istate->cache_alloc);
-	memcpy(istate->cache, si->base->cache,
-	       sizeof(*istate->cache) * istate->cache_nr);
+	COPY_ARRAY(istate->cache, si->base->cache, istate->cache_nr);
 
 	si->nr_deletions = 0;
 	si->nr_replacements = 0;
-- 
2.10.0

Re: [PATCH 6/7] use COPY_ARRAY

[René Scharfe]

Ha, can't count.  It should be [PATCH 2/2] of course.

René

Re: [PATCH 1/2] add COPY_ARRAY

[Jeff King]

On Sun, Sep 25, 2016 at 09:15:42AM +0200, René Scharfe wrote:

> Add COPY_ARRAY, a safe and convenient helper for copying arrays,
> complementing ALLOC_ARRAY and REALLOC_ARRAY.  Users just specify source,
> destination and the number of elements; the size of an element is
> inferred automatically.

Seems like a fairly readable construct to have.

> It checks if the multiplication of size and element count overflows.
> The inferred size is passed first to st_mult, which allows the division
> there to be done at compilation time.

I wonder if this actually stops any real overflows. My goal with
ALLOC_ARRAY, etc, was to catch these at the malloc stage (which is the
really dangerous part, because we don't want to under-allocate). So the
first hunk of your patch is:

        ALLOC_ARRAY(result, count + 1);
-       memcpy(result, pathspec, count * sizeof(const char *));
+       COPY_ARRAY(result, pathspec, count);

which clearly cannot trigger the st_mult() check, because we would have
done so in the ALLOC_ARRAY call[1].

Other calls are not so obvious, but in general I would expect the
allocation step to be doing this check. If we missed one, then it's
possible that this macro could detect it and prevent a problem. But it
seems like the wrong time to check. The allocation is buggy, and we'd
have to just get lucky to be using COPY_ARRAY(). And I don't even mean
"lucky that we switched to COPY_ARRAY from memcpy for this callsite".
There are lots of sites that allocate and then fill the array one by
one, without ever computing the full size again. So allocation is the
only sensible place to enforce integer overflow.

So I'm not sold on this providing any real integer overflow safety. But
I do otherwise like it, as it drops the extra "sizeof" which has to
repeat either the variable name or the type).

-Peff

[1] Actually, this particular example probably should be using
    st_add(count, 1), though it's likely not a problem in practice
    ("count" is an int, so you cannot easily overflow it back to 0 by
    incrementing 1, though of course overflowing it at all is undefined
    behavior).

Re: [PATCH 1/2] add COPY_ARRAY

[René Scharfe]

Am 25.09.2016 um 09:41 schrieb Jeff King:
> On Sun, Sep 25, 2016 at 09:15:42AM +0200, René Scharfe wrote:
>> It checks if the multiplication of size and element count overflows.
>> The inferred size is passed first to st_mult, which allows the division
>> there to be done at compilation time.
>
> I wonder if this actually stops any real overflows. My goal with
> ALLOC_ARRAY, etc, was to catch these at the malloc stage (which is the
> really dangerous part, because we don't want to under-allocate). So the
> first hunk of your patch is:
>
>         ALLOC_ARRAY(result, count + 1);
> -       memcpy(result, pathspec, count * sizeof(const char *));
> +       COPY_ARRAY(result, pathspec, count);
>
> which clearly cannot trigger the st_mult() check, because we would have
> done so in the ALLOC_ARRAY call[1].
>
> Other calls are not so obvious, but in general I would expect the
> allocation step to be doing this check. If we missed one, then it's
> possible that this macro could detect it and prevent a problem. But it
> seems like the wrong time to check. The allocation is buggy, and we'd
> have to just get lucky to be using COPY_ARRAY(). And I don't even mean
> "lucky that we switched to COPY_ARRAY from memcpy for this callsite".
> There are lots of sites that allocate and then fill the array one by
> one, without ever computing the full size again. So allocation is the
> only sensible place to enforce integer overflow.
>
> So I'm not sold on this providing any real integer overflow safety. But
> I do otherwise like it, as it drops the extra "sizeof" which has to
> repeat either the variable name or the type).

Well, yes, checking if the destination object is big enough requires 
calculating the size of the source object and thus should include a 
check for multiplication overflow already.  Note the word "should". :) 
If that's the case then a smart compiler could elide the duplicate 
check.  Keeping the check in COPY_ARRAY reduces the assumptions we have 
make about its use as well as any previous checks and doesn't cost much.

René