threads / patch / 44151

patch, 2 partsadd COPY_ARRAY

Subject: [PATCH 1/2] add COPY_ARRAY

## tl;dr

5 messages between Sep 25, 2016 and Sep 25, 2016. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

René Scharfe· Sep 25, 2016, 07:15 UTC · lore

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(+)

Show changes to git-compat-util.h +8 −0

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

René Scharfe· Sep 25, 2016, 07:24 UTC · re: René Scharfe · lore

[PATCH 6/7] use COPY_ARRAY

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

Show changes to 6 files +32 −9

builtin/mv.c, commit.c, contrib/coccinelle/array.cocci, pack-revindex.c, pathspec.c, split-index.c

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

René Scharfe· Sep 25, 2016, 07:26 UTC · re: René Scharfe · lore

Re: [PATCH 6/7] use COPY_ARRAY

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

René

Jeff King· Sep 25, 2016, 07:41 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] add COPY_ARRAY

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).

René Scharfe· Sep 25, 2016, 08:58 UTC · re: Jeff King · lore

Re: [PATCH 1/2] add COPY_ARRAY

Am 25.09.2016 um 09:41 schrieb Jeff King:

Show 30 quoted lines

> 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é