[PATCH 0/3] COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY

[PATCH 0/3] COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY

[René Scharfe]

Tighten the type check of COPY_ARRAY and MOVE_ARRAY.  Then add DUP_ARRAY
for making a shallow copy without repeating destination and element
count, which inherits the stricter check.

  do full type check in COPY_ARRAY and MOVE_ARRAY
  add DUP_ARRAY
  use DUP_ARRAY

 attr.c                         | 3 +--
 builtin/am.c                   | 3 +--
 commit-graph.c                 | 3 +--
 commit-reach.c                 | 3 +--
 compat/mingw.c                 | 3 +--
 contrib/coccinelle/array.cocci | 7 +++++++
 git-compat-util.h              | 7 +++++++
 parse-options.c                | 3 +--
 pathspec.c                     | 6 ++----
 9 files changed, 22 insertions(+), 16 deletions(-)

--
2.39.0

[PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[René Scharfe]

Extend the element size comparison between source and destination with
a full type check using an assignment.  It is not actually evaluated and
even optimized out, but compilers check the types before getting to that
point, and report mismatches.

The stricter check improves safety, as it catches attempts to copy
between different types that happen to have the same size.  The size
check is still needed to avoid allowing copies from a array with a
smaller element type to a bigger one, e.g. from a char array to an int
array, which would be allowed by the assignment check alone.

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

diff --git a/git-compat-util.h b/git-compat-util.h
index 76e4b11131..8d04832988 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1094,6 +1094,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
 #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)) + \
+	(0 ? (*(dst) = *(src), 0) : 0) + \
 	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
 static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
 {
@@ -1102,6 +1103,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
 }

 #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \
+	(0 ? (*(dst) = *(src), 0) : 0) + \
 	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
 static inline void move_array(void *dst, const void *src, size_t n, size_t size)
 {
--
2.39.0

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Extend the element size comparison between source and destination with
> a full type check using an assignment.  It is not actually evaluated and
> even optimized out, but compilers check the types before getting to that
> point, and report mismatches.

It is not actually evaluated and is (even) optimized out, which is a
neat and clever trick.  The above made me read it twice, though, as
I misread on my first read that you are saying that the code is left
in the binary but in an unreachable form (i.e. "it is not (evaluated
or optimized out"), though.

> The stricter check improves safety, as it catches attempts to copy
> between different types that happen to have the same size.  The size
> check is still needed to avoid allowing copies from a array with a
> smaller element type to a bigger one, e.g. from a char array to an int
> array, which would be allowed by the assignment check alone.

Do you mean

	int *ip;
	char *cp;

	(0 ? (*(ip) = *(cp), 0) : 0); /* silently allowed */
	(0 ? (*(cp) = *(ip), 0) : 0); /* truncation noticed */

Presumably we cannot catch
	
	int *ip;
	unsinged *up;

	(0 ? (*(ip) = *(up), 0) : 0);
	(0 ? (*(up) = *(ip), 0) : 0);

with the approach?  I think what you ideally want to enforce is that
typeof(dst) is exactly typeof(src), or typeof(src) sans constness
(i.e. you can populate non-const array from a const template)?

>  #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
> +	(0 ? (*(dst) = *(src), 0) : 0) + \
>  	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
>  static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
>  {
> @@ -1102,6 +1103,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
>  }
>
>  #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \
> +	(0 ? (*(dst) = *(src), 0) : 0) + \
>  	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
>  static inline void move_array(void *dst, const void *src, size_t n, size_t size)
>  {
> --
> 2.39.0

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...  I think what you ideally want to enforce is that
> typeof(dst) is exactly typeof(src), or typeof(src) sans constness
> (i.e. you can populate non-const array from a const template)?

IOW, I am wondering if something like this is an improvement.

 git-compat-util.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git c/git-compat-util.h w/git-compat-util.h
index a76d0526f7..be435615f0 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -97,8 +97,13 @@ struct strbuf;
 # define BARF_UNLESS_AN_ARRAY(arr)						\
 	BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \
 							   __typeof__(&(arr)[0])))
+# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \
+	(BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \
+							    __typeof__(dst))) + \
+			      (0 ? *(dst) = *(src) : 0))
 #else
 # define BARF_UNLESS_AN_ARRAY(arr) 0
+# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0))
 #endif
 /*
  * ARRAY_SIZE - get the number of elements in a visible array

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[René Scharfe]

Am 01.01.23 um 04:59 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> ...  I think what you ideally want to enforce is that
>> typeof(dst) is exactly typeof(src), or typeof(src) sans constness
>> (i.e. you can populate non-const array from a const template)?

Yes.

Moving the type check shared between COPY_ARRAY and MOVE_ARRAY to a new
macro is a good idea.

Using compiler extensions when available, as we already do for other
purposes, is a good idea as well.  I managed to ignore the existing use
somehow.

>
> IOW, I am wondering if something like this is an improvement.
>
>  git-compat-util.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git c/git-compat-util.h w/git-compat-util.h
> index a76d0526f7..be435615f0 100644
> --- c/git-compat-util.h
> +++ w/git-compat-util.h
> @@ -97,8 +97,13 @@ struct strbuf;
>  # define BARF_UNLESS_AN_ARRAY(arr)						\
>  	BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \
>  							   __typeof__(&(arr)[0])))
> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \
> +	(BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \
> +							    __typeof__(dst))) + \
> +			      (0 ? *(dst) = *(src) : 0))
>  #else
>  # define BARF_UNLESS_AN_ARRAY(arr) 0
> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0))
>  #endif
>  /*
>   * ARRAY_SIZE - get the number of elements in a visible array

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[René Scharfe]

Am 01.01.23 um 08:41 schrieb René Scharfe:
> Am 01.01.23 um 04:59 schrieb Junio C Hamano:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> ...  I think what you ideally want to enforce is that
>>> typeof(dst) is exactly typeof(src), or typeof(src) sans constness
>>> (i.e. you can populate non-const array from a const template)?
>
> Yes.
>
> Moving the type check shared between COPY_ARRAY and MOVE_ARRAY to a new
> macro is a good idea.
>
> Using compiler extensions when available, as we already do for other
> purposes, is a good idea as well.  I managed to ignore the existing use
> somehow.

On second thought, what do we gain by using __builtin_types_compatible_p
here?  Does it catch cases that the assignment check plus the element
size check wouldn't?

>
>>
>> IOW, I am wondering if something like this is an improvement.
>>
>>  git-compat-util.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git c/git-compat-util.h w/git-compat-util.h
>> index a76d0526f7..be435615f0 100644
>> --- c/git-compat-util.h
>> +++ w/git-compat-util.h
>> @@ -97,8 +97,13 @@ struct strbuf;
>>  # define BARF_UNLESS_AN_ARRAY(arr)						\
>>  	BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \
>>  							   __typeof__(&(arr)[0])))
>> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) \
>> +	(BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(src), \
>> +							    __typeof__(dst))) + \

We actually need to compare the types of the elements here, because
otherwise we'd disallow copies between arrays (int arr[7]) and pointers
(int *p), which would be unnecessarily strict.

>> +			      (0 ? *(dst) = *(src) : 0))

You dropped the ", 0" after the assignment, but we need it to make the
type of this expression int instead of whatever type *dst and *src have.

>>  #else
>>  # define BARF_UNLESS_AN_ARRAY(arr) 0
>> +# define ARRAYS_COPYABLE_OR_ZERO(src,dst) (0 ? *(dst) = *(src) : 0))
>>  #endif
>>  /*
>>   * ARRAY_SIZE - get the number of elements in a visible array

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On second thought, what do we gain by using __builtin_types_compatible_p
> here?  Does it catch cases that the assignment check plus the element
> size check wouldn't?

I was reacting to this part of an earlier message in the thread:

Presumably we cannot catch
	
	int *ip;
	unsigned *up;

	(0 ? (*(ip) = *(up), 0) : 0);
	(0 ? (*(up) = *(ip), 0) : 0);

with the approach?

i.e. *ip and *up are of the same size.  Would the assignment check
trigger?

> We actually need to compare the types of the elements here, because
> otherwise we'd disallow copies between arrays (int arr[7]) and pointers
> (int *p), which would be unnecessarily strict.

Yup.  Thanks for inferring what I meant to give, despite the two
typoes (the other one is ", 0").

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[René Scharfe]

Am 01.01.23 um 13:11 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> On second thought, what do we gain by using __builtin_types_compatible_p
>> here?  Does it catch cases that the assignment check plus the element
>> size check wouldn't?
>
> I was reacting to this part of an earlier message in the thread:
>
> Presumably we cannot catch
>
> 	int *ip;
> 	unsigned *up;
>
> 	(0 ? (*(ip) = *(up), 0) : 0);
> 	(0 ? (*(up) = *(ip), 0) : 0);
>
> with the approach?
>
> i.e. *ip and *up are of the same size.  Would the assignment check
> trigger?

Ah, right, __builtin_types_compatible_p returns 0 in this case and an
assignment is silently allowed.

René

Re: [PATCH 1/3] do full type check in COPY_ARRAY and MOVE_ARRAY

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Ah, right, __builtin_types_compatible_p returns 0 in this case and an
> assignment is silently allowed.

Thanks.  There is another thing I forgot to mention.  I think the
side that can use __builtin_types_compatible_p() can lose the
assignment check (what I wrote had the GCC extension in addition to
the assignment check instead), i.e. the assignment check from your
original patch can be considered as a fallback position for
compilers without the GCC extension.

[PATCH 2/3] add DUP_ARRAY

[René Scharfe]

Add a macro for allocating and populating a shallow copy of an array.
It is intended to replace a sequence like this:

   ALLOC_ARRAY(dst, n);
   COPY_ARRAY(dst, src, n);

With the less repetitive:

   DUP_ARRAY(dst, src, n);

It checks whether the types of source and destination are compatible to
ensure the copy can be used safely.

An easier alternative would be to only consider the source and return
a void pointer, that could be used like this:

   dst = ARRAY_DUP(src, n);

That would be more versatile, as it could be used in declarations as
well.  Making it type-safe would require the use of typeof from C23,
though, as far as I can see.

So use the first variant, whose safety requires no compiler extensions
or future features.

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

diff --git a/git-compat-util.h b/git-compat-util.h
index 8d04832988..29f4f699b5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1111,6 +1111,11 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
 		memmove(dst, src, st_mult(size, n));
 }

+#define DUP_ARRAY(dst, src, n) do { \
+	size_t dup_array_n_ = (n); \
+	COPY_ARRAY(ALLOC_ARRAY((dst), dup_array_n_), (src), dup_array_n_); \
+} while (0)
+
 /*
  * These functions help you allocate structs with flex arrays, and copy
  * the data directly into the array. For example, if you had:
--
2.39.0

[PATCH 3/3] use DUP_ARRAY

[René Scharfe]

Add a semantic patch to replace ALLOC_ARRAY+COPY_ARRAY with DUP_ARRAY
to reduce code duplication and apply its results.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 attr.c                         | 3 +--
 builtin/am.c                   | 3 +--
 commit-graph.c                 | 3 +--
 commit-reach.c                 | 3 +--
 compat/mingw.c                 | 3 +--
 contrib/coccinelle/array.cocci | 7 +++++++
 parse-options.c                | 3 +--
 pathspec.c                     | 6 ++----
 8 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/attr.c b/attr.c
index 42ad6de8c7..e4e304a826 100644
--- a/attr.c
+++ b/attr.c
@@ -599,8 +599,7 @@ struct attr_check *attr_check_dup(const struct attr_check *check)

 	ret->nr = check->nr;
 	ret->alloc = check->alloc;
-	ALLOC_ARRAY(ret->items, ret->nr);
-	COPY_ARRAY(ret->items, check->items, ret->nr);
+	DUP_ARRAY(ret->items, check->items, ret->nr);

 	return ret;
 }
diff --git a/builtin/am.c b/builtin/am.c
index dddf1b9af0..eee06bbb6c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1489,8 +1489,7 @@ static int run_apply(const struct am_state *state, const char *index_file)
 	 * apply_opts.v keeps referencing the allocated strings for
 	 * strvec_clear() to release.
 	 */
-	ALLOC_ARRAY(apply_argv, apply_opts.nr);
-	COPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);
+	DUP_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);

 	opts_left = apply_parse_options(apply_opts.nr, apply_argv,
 					&apply_state, &force_apply, &options,
diff --git a/commit-graph.c b/commit-graph.c
index a7d8755932..c11b59f28b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1594,8 +1594,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
 			_("Computing commit changed paths Bloom filters"),
 			ctx->commits.nr);

-	ALLOC_ARRAY(sorted_commits, ctx->commits.nr);
-	COPY_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);
+	DUP_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);

 	if (ctx->order_by_pack)
 		QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);
diff --git a/commit-reach.c b/commit-reach.c
index c226ee3da4..2e33c599a8 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -245,8 +245,7 @@ static int remove_redundant_with_gen(struct repository *r,
 	 * min_gen_pos points to the current position within 'array'
 	 * that is not yet known to be STALE.
 	 */
-	ALLOC_ARRAY(sorted, cnt);
-	COPY_ARRAY(sorted, array, cnt);
+	DUP_ARRAY(sorted, array, cnt);
 	QSORT(sorted, cnt, compare_commits_by_gen);
 	min_generation = commit_graph_generation(sorted[0]);

diff --git a/compat/mingw.c b/compat/mingw.c
index d614f156df..ddc5dd0377 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1396,8 +1396,7 @@ static wchar_t *make_environment_block(char **deltaenv)
 			p += s;
 		}

-		ALLOC_ARRAY(result, size);
-		COPY_ARRAY(result, wenv, size);
+		DUP_ARRAY(result, wenv, size);
 		FreeEnvironmentStringsW(wenv);
 		return result;
 	}
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index aa75937950..27a3b479c9 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -94,3 +94,10 @@ expression n != 1;
 @@
 - ptr = xcalloc(n, \( sizeof(*ptr) \| sizeof(T) \) )
 + CALLOC_ARRAY(ptr, n)
+
+@@
+expression dst, src, n;
+@@
+-ALLOC_ARRAY(dst, n);
+-COPY_ARRAY(dst, src, n);
++DUP_ARRAY(dst, src, n);
diff --git a/parse-options.c b/parse-options.c
index a1ec932f0f..fd4743293f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -702,8 +702,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 	if (!nr_aliases)
 		return NULL;

-	ALLOC_ARRAY(newopt, nr + 1);
-	COPY_ARRAY(newopt, options, nr + 1);
+	DUP_ARRAY(newopt, options, nr + 1);

 	/* each alias has two string pointers and NULL */
 	CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));
diff --git a/pathspec.c b/pathspec.c
index 46e77a85fe..e038481dc4 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -681,8 +681,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 	int i, j;

 	*dst = *src;
-	ALLOC_ARRAY(dst->items, dst->nr);
-	COPY_ARRAY(dst->items, src->items, dst->nr);
+	DUP_ARRAY(dst->items, src->items, dst->nr);

 	for (i = 0; i < dst->nr; i++) {
 		struct pathspec_item *d = &dst->items[i];
@@ -691,8 +690,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 		d->match = xstrdup(s->match);
 		d->original = xstrdup(s->original);

-		ALLOC_ARRAY(d->attr_match, d->attr_match_nr);
-		COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);
+		DUP_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);
 		for (j = 0; j < d->attr_match_nr; j++) {
 			const char *value = s->attr_match[j].value;
 			d->attr_match[j].value = xstrdup_or_null(value);
--
2.39.0

[PATCH v2 0/4] COPY_ARRAY, MOVE_ARRAY, DUP_ARRAY

[René Scharfe]

Changes since v1:
- add macro BARF_UNLESS_COPYABLE
- use __builtin_types_compatible_p if available

  factor out BARF_UNLESS_COPYABLE
  do full type check in BARF_UNLESS_COPYABLE
  add DUP_ARRAY
  use DUP_ARRAY

 attr.c                         |  3 +--
 builtin/am.c                   |  3 +--
 commit-graph.c                 |  3 +--
 commit-reach.c                 |  3 +--
 compat/mingw.c                 |  3 +--
 contrib/coccinelle/array.cocci |  7 +++++++
 git-compat-util.h              | 15 +++++++++++++--
 parse-options.c                |  3 +--
 pathspec.c                     |  6 ++----
 9 files changed, 28 insertions(+), 18 deletions(-)

Range-Diff gegen v1:
1:  052b9c103d < -:  ---------- do full type check in COPY_ARRAY and MOVE_ARRAY
-:  ---------- > 1:  f50df9208f factor out BARF_UNLESS_COPYABLE
-:  ---------- > 2:  6a98a8f2a2 do full type check in BARF_UNLESS_COPYABLE
2:  fb5544fc51 ! 3:  757baca245 add DUP_ARRAY
    @@ Commit message
            dst = ARRAY_DUP(src, n);

         That would be more versatile, as it could be used in declarations as
    -    well.  Making it type-safe would require the use of typeof from C23,
    -    though.
    +    well.  Making it type-safe would require the use of typeof_unqual from
    +    C23, though.

         So use the safe and compatible variant for now.

3:  bfbf085ac3 = 4:  435fbd8e92 use DUP_ARRAY
--
2.39.0

[PATCH v2 1/4] factor out BARF_UNLESS_COPYABLE

[René Scharfe]

Move the common basic element type check of COPY_ARRAY and MOVE_ARRAY to
a new macro.  This reduces code duplication and simplifies adding more
elaborate checks.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 76e4b11131..940d03150c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1093,8 +1093,11 @@ int xstrncmpz(const char *s, const char *t, size_t len);
 #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))

+#define BARF_UNLESS_COPYABLE(dst, src) \
+	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))
+
 #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
-	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
+	BARF_UNLESS_COPYABLE((dst), (src)))
 static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
 {
 	if (n)
@@ -1102,7 +1105,7 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
 }

 #define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \
-	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
+	BARF_UNLESS_COPYABLE((dst), (src)))
 static inline void move_array(void *dst, const void *src, size_t n, size_t size)
 {
 	if (n)
--
2.39.0

[PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE

[René Scharfe]

Use __builtin_types_compatible_p to perform a full type check if
possible.  Otherwise fall back to the old size comparison, but add a
non-evaluated assignment to catch more type mismatches.  It doesn't flag
copies between arrays with different signedness, but that's as close to
a full type check as it gets without the builtin, as far as I can see.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 940d03150c..e81bb14fc9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -97,8 +97,14 @@ struct strbuf;
 # define BARF_UNLESS_AN_ARRAY(arr)						\
 	BUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \
 							   __typeof__(&(arr)[0])))
+# define BARF_UNLESS_COPYABLE(dst, src) \
+	BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \
+							  __typeof__(*(src))))
 #else
 # define BARF_UNLESS_AN_ARRAY(arr) 0
+# define BARF_UNLESS_COPYABLE(dst, src) \
+	BUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \
+				 sizeof(*(dst)) == sizeof(*(src)))
 #endif
 /*
  * ARRAY_SIZE - get the number of elements in a visible array
@@ -1093,9 +1099,6 @@ int xstrncmpz(const char *s, const char *t, size_t len);
 #define CALLOC_ARRAY(x, alloc) (x) = xcalloc((alloc), sizeof(*(x)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))

-#define BARF_UNLESS_COPYABLE(dst, src) \
-	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))
-
 #define COPY_ARRAY(dst, src, n) copy_array((dst), (src), (n), sizeof(*(dst)) + \
 	BARF_UNLESS_COPYABLE((dst), (src)))
 static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
--
2.39.0

Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Use __builtin_types_compatible_p to perform a full type check if
> possible.  Otherwise fall back to the old size comparison, but add a
> non-evaluated assignment to catch more type mismatches.  It doesn't flag
> copies between arrays with different signedness, but that's as close to
> a full type check as it gets without the builtin, as far as I can see.

This seems to unfortunately break builds for compat/mingw.c

cf. https://github.com/git/git/actions/runs/3865788736/jobs/6589504628#step:4:374

   1848 |                 COPY_ARRAY(&argv2[1], &argv[1], argc);

where the two arrays are "char *const *argv" in the parameter list, and
a local variable

#ifndef _MSC_VER
		const
#endif
		char **argv2;

It seems that (const char **) and (char **) are compatible but the
pointers themselves being const breaks the type compatibility?
Perhaps the latter should be "(optionally const) char *const *argv2"?

Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE

[René Scharfe]

Am 08.01.2023 um 08:28 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Use __builtin_types_compatible_p to perform a full type check if
>> possible.  Otherwise fall back to the old size comparison, but add a
>> non-evaluated assignment to catch more type mismatches.  It doesn't flag
>> copies between arrays with different signedness, but that's as close to
>> a full type check as it gets without the builtin, as far as I can see.
>
> This seems to unfortunately break builds for compat/mingw.c
>
> cf. https://github.com/git/git/actions/runs/3865788736/jobs/6589504628#step:4:374
>
>    1848 |                 COPY_ARRAY(&argv2[1], &argv[1], argc);
>
> where the two arrays are "char *const *argv" in the parameter list, and
> a local variable
>
> #ifndef _MSC_VER
> 		const
> #endif
> 		char **argv2;
>
> It seems that (const char **) and (char **) are compatible but the
> pointers themselves being const breaks the type compatibility?
> Perhaps the latter should be "(optionally const) char *const *argv2"?

We compare the types of the elements, so effectively we do this:

   __builtin_types_compatible_p(__typeof__(const char *),  __typeof__(char *))

... which returns 0.

We can remove the const like we already do for Visual Studio.  But
then we have to add two casts when passing on argv2, like in
mingw_execv(), because adding a const to a pointer of a pointer
must be done explicitly in C (even though Visual Studio seems to
do it implicitly without complaining).  Feels a bit silly. :-|

--- >8 ---
Subject: [PATCH 1.5/4] mingw: make argv2 in try_shell_exec() non-const

Prepare for a stricter type check in COPY_ARRAY by removing the const
qualifier of argv2, like we already do to placate Visual Studio.  We
have to add it back using explicit casts when actually using the
variable, unfortunately, because GCC (rightly) refuses to add it
implicitly.  Similar casts are already used in mingw_execv().

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mingw.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index d614f156df..e131eb9b07 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1839,16 +1839,13 @@ static int try_shell_exec(const char *cmd, char *const *argv)
 	if (prog) {
 		int exec_id;
 		int argc = 0;
-#ifndef _MSC_VER
-		const
-#endif
 		char **argv2;
 		while (argv[argc]) argc++;
 		ALLOC_ARRAY(argv2, argc + 1);
 		argv2[0] = (char *)cmd;	/* full path to the script file */
 		COPY_ARRAY(&argv2[1], &argv[1], argc);
-		exec_id = trace2_exec(prog, argv2);
-		pid = mingw_spawnv(prog, argv2, 1);
+		exec_id = trace2_exec(prog, (const char **)argv2);
+		pid = mingw_spawnv(prog, (const char **)argv2, 1);
 		if (pid >= 0) {
 			int status;
 			if (waitpid(pid, &status, 0) < 0)
--
2.38.1.windows.1

Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> We compare the types of the elements, so effectively we do this:
>
>    __builtin_types_compatible_p(__typeof__(const char *),  __typeof__(char *))
>
> ... which returns 0.

True.  I wonder if (const const char *) and (const char *) are
deemed compatible?  Even if so, probably we cannot write

	__builtin_types_compatible_p(const __typeof__(*(dst)),
				     const __typeof__(*(src)))

so that line of thoguht would lead nowhere X-<.

> We can remove the const like we already do for Visual Studio.  But
> then we have to add two casts when passing on argv2, like in
> mingw_execv(), because adding a const to a pointer of a pointer
> must be done explicitly in C (even though Visual Studio seems to
> do it implicitly without complaining).  Feels a bit silly. :-|

Indeed.  Let's see what folks, whom "git blame" tells us to be area
experts around here, think.  The "if _MSC, add const" was added in
12fb9bd8 (msvc: mark a variable as non-const, 2019-06-19) by JeffH,
and try_shell_exec() function itself came from f1a4dfb8 (Windows:
Wrap execve so that shell scripts can be invoked., 2007-12-04),
added by J6t.

> --- >8 ---
> Subject: [PATCH 1.5/4] mingw: make argv2 in try_shell_exec() non-const
>
> Prepare for a stricter type check in COPY_ARRAY by removing the const
> qualifier of argv2, like we already do to placate Visual Studio.  We
> have to add it back using explicit casts when actually using the
> variable, unfortunately, because GCC (rightly) refuses to add it
> implicitly.  Similar casts are already used in mingw_execv().
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/mingw.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index d614f156df..e131eb9b07 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1839,16 +1839,13 @@ static int try_shell_exec(const char *cmd, char *const *argv)
>  	if (prog) {
>  		int exec_id;
>  		int argc = 0;
> -#ifndef _MSC_VER
> -		const
> -#endif
>  		char **argv2;
>  		while (argv[argc]) argc++;
>  		ALLOC_ARRAY(argv2, argc + 1);
>  		argv2[0] = (char *)cmd;	/* full path to the script file */
>  		COPY_ARRAY(&argv2[1], &argv[1], argc);
> -		exec_id = trace2_exec(prog, argv2);
> -		pid = mingw_spawnv(prog, argv2, 1);
> +		exec_id = trace2_exec(prog, (const char **)argv2);
> +		pid = mingw_spawnv(prog, (const char **)argv2, 1);
>  		if (pid >= 0) {
>  			int status;
>  			if (waitpid(pid, &status, 0) < 0)
> --
> 2.38.1.windows.1

Re: [PATCH v2 2/4] do full type check in BARF_UNLESS_COPYABLE

[Jeff Hostetler]

On 1/8/23 11:27 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> We compare the types of the elements, so effectively we do this:
>>
>>     __builtin_types_compatible_p(__typeof__(const char *),  __typeof__(char *))
>>
>> ... which returns 0.
> 
> True.  I wonder if (const const char *) and (const char *) are
> deemed compatible?  Even if so, probably we cannot write
> 
> 	__builtin_types_compatible_p(const __typeof__(*(dst)),
> 				     const __typeof__(*(src)))
> 
> so that line of thoguht would lead nowhere X-<.
> 
>> We can remove the const like we already do for Visual Studio.  But
>> then we have to add two casts when passing on argv2, like in
>> mingw_execv(), because adding a const to a pointer of a pointer
>> must be done explicitly in C (even though Visual Studio seems to
>> do it implicitly without complaining).  Feels a bit silly. :-|
> 
> Indeed.  Let's see what folks, whom "git blame" tells us to be area
> experts around here, think.  The "if _MSC, add const" was added in
> 12fb9bd8 (msvc: mark a variable as non-const, 2019-06-19) by JeffH,
> and try_shell_exec() function itself came from f1a4dfb8 (Windows:
> Wrap execve so that shell scripts can be invoked., 2007-12-04),
> added by J6t.

I added the ifNdef.  The existing code already had the const
that MSVC didn't like.  I don't remember why I didn't just remove
the const completely.  If I had to guess I'd say that dropping
probably caused a small cascade of caller/callee type changes
and that would have distracted from the focus of my patch series
at the time.

Personally, I think we should just remove the const from all
versions.

Thanks,
Jeff


...
>>   		int exec_id;
>>   		int argc = 0;
>> -#ifndef _MSC_VER
>> -		const
>> -#endif
>>   		char **argv2;
>>   		while (argv[argc]) argc++;
>>   		ALLOC_ARRAY(argv2, argc + 1);
>>   		argv2[0] = (char *)cmd;	/* full path to the script file */
>>   		COPY_ARRAY(&argv2[1], &argv[1], argc);
>> -		exec_id = trace2_exec(prog, argv2);
>> -		pid = mingw_spawnv(prog, argv2, 1);
>> +		exec_id = trace2_exec(prog, (const char **)argv2);
>> +		pid = mingw_spawnv(prog, (const char **)argv2, 1);
>>   		if (pid >= 0) {
>>   			int status;
>>   			if (waitpid(pid, &status, 0) < 0)
>> --
>> 2.38.1.windows.1

[PATCH v2 3/4] add DUP_ARRAY

[René Scharfe]

Add a macro for allocating and populating a shallow copy of an array.
It is intended to replace a sequence like this:

   ALLOC_ARRAY(dst, n);
   COPY_ARRAY(dst, src, n);

With the less repetitve:

   DUP_ARRAY(dst, src, n);

It checks whether the types of source and destination are compatible to
ensure the copy can be used safely.

An easier alternative would be to only consider the source and return
a void pointer, that could be used like this:

   dst = ARRAY_DUP(src, n);

That would be more versatile, as it could be used in declarations as
well.  Making it type-safe would require the use of typeof_unqual from
C23, though.

So use the safe and compatible variant for now.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Why typeof_unqual and not just typeof?  To remove const when duplicating
a const array.

 git-compat-util.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index e81bb14fc9..44abb240ae 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1115,6 +1115,11 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
 		memmove(dst, src, st_mult(size, n));
 }

+#define DUP_ARRAY(dst, src, n) do { \
+	size_t dup_array_n_ = (n); \
+	COPY_ARRAY(ALLOC_ARRAY((dst), dup_array_n_), (src), dup_array_n_); \
+} while (0)
+
 /*
  * These functions help you allocate structs with flex arrays, and copy
  * the data directly into the array. For example, if you had:
--
2.39.0

[PATCH v2 4/4] use DUP_ARRAY

[René Scharfe]

Add a semantic patch for replace ALLOC_ARRAY+COPY_ARRAY with DUP_ARRAY
to reduce code duplication and apply its results.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 attr.c                         | 3 +--
 builtin/am.c                   | 3 +--
 commit-graph.c                 | 3 +--
 commit-reach.c                 | 3 +--
 compat/mingw.c                 | 3 +--
 contrib/coccinelle/array.cocci | 7 +++++++
 parse-options.c                | 3 +--
 pathspec.c                     | 6 ++----
 8 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/attr.c b/attr.c
index 42ad6de8c7..e4e304a826 100644
--- a/attr.c
+++ b/attr.c
@@ -599,8 +599,7 @@ struct attr_check *attr_check_dup(const struct attr_check *check)

 	ret->nr = check->nr;
 	ret->alloc = check->alloc;
-	ALLOC_ARRAY(ret->items, ret->nr);
-	COPY_ARRAY(ret->items, check->items, ret->nr);
+	DUP_ARRAY(ret->items, check->items, ret->nr);

 	return ret;
 }
diff --git a/builtin/am.c b/builtin/am.c
index dddf1b9af0..eee06bbb6c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1489,8 +1489,7 @@ static int run_apply(const struct am_state *state, const char *index_file)
 	 * apply_opts.v keeps referencing the allocated strings for
 	 * strvec_clear() to release.
 	 */
-	ALLOC_ARRAY(apply_argv, apply_opts.nr);
-	COPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);
+	DUP_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);

 	opts_left = apply_parse_options(apply_opts.nr, apply_argv,
 					&apply_state, &force_apply, &options,
diff --git a/commit-graph.c b/commit-graph.c
index a7d8755932..c11b59f28b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1594,8 +1594,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
 			_("Computing commit changed paths Bloom filters"),
 			ctx->commits.nr);

-	ALLOC_ARRAY(sorted_commits, ctx->commits.nr);
-	COPY_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);
+	DUP_ARRAY(sorted_commits, ctx->commits.list, ctx->commits.nr);

 	if (ctx->order_by_pack)
 		QSORT(sorted_commits, ctx->commits.nr, commit_pos_cmp);
diff --git a/commit-reach.c b/commit-reach.c
index c226ee3da4..2e33c599a8 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -245,8 +245,7 @@ static int remove_redundant_with_gen(struct repository *r,
 	 * min_gen_pos points to the current position within 'array'
 	 * that is not yet known to be STALE.
 	 */
-	ALLOC_ARRAY(sorted, cnt);
-	COPY_ARRAY(sorted, array, cnt);
+	DUP_ARRAY(sorted, array, cnt);
 	QSORT(sorted, cnt, compare_commits_by_gen);
 	min_generation = commit_graph_generation(sorted[0]);

diff --git a/compat/mingw.c b/compat/mingw.c
index d614f156df..ddc5dd0377 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1396,8 +1396,7 @@ static wchar_t *make_environment_block(char **deltaenv)
 			p += s;
 		}

-		ALLOC_ARRAY(result, size);
-		COPY_ARRAY(result, wenv, size);
+		DUP_ARRAY(result, wenv, size);
 		FreeEnvironmentStringsW(wenv);
 		return result;
 	}
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index aa75937950..27a3b479c9 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -94,3 +94,10 @@ expression n != 1;
 @@
 - ptr = xcalloc(n, \( sizeof(*ptr) \| sizeof(T) \) )
 + CALLOC_ARRAY(ptr, n)
+
+@@
+expression dst, src, n;
+@@
+-ALLOC_ARRAY(dst, n);
+-COPY_ARRAY(dst, src, n);
++DUP_ARRAY(dst, src, n);
diff --git a/parse-options.c b/parse-options.c
index a1ec932f0f..fd4743293f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -702,8 +702,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 	if (!nr_aliases)
 		return NULL;

-	ALLOC_ARRAY(newopt, nr + 1);
-	COPY_ARRAY(newopt, options, nr + 1);
+	DUP_ARRAY(newopt, options, nr + 1);

 	/* each alias has two string pointers and NULL */
 	CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));
diff --git a/pathspec.c b/pathspec.c
index 46e77a85fe..e038481dc4 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -681,8 +681,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 	int i, j;

 	*dst = *src;
-	ALLOC_ARRAY(dst->items, dst->nr);
-	COPY_ARRAY(dst->items, src->items, dst->nr);
+	DUP_ARRAY(dst->items, src->items, dst->nr);

 	for (i = 0; i < dst->nr; i++) {
 		struct pathspec_item *d = &dst->items[i];
@@ -691,8 +690,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 		d->match = xstrdup(s->match);
 		d->original = xstrdup(s->original);

-		ALLOC_ARRAY(d->attr_match, d->attr_match_nr);
-		COPY_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);
+		DUP_ARRAY(d->attr_match, s->attr_match, d->attr_match_nr);
 		for (j = 0; j < d->attr_match_nr; j++) {
 			const char *value = s->attr_match[j].value;
 			d->attr_match[j].value = xstrdup_or_null(value);
--
2.39.0