From 09884f352eb36cf2579d819595e9b7e1656a28b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 8 Jan 2023 11:10:59 +0100 Subject: [PATCH 1/5] mingw: make argv2 in try_shell_exec() non-const MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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) From 1891846fa4d439be7f9a1a32c062f62cd863df2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 1 Jan 2023 22:08:53 +0100 Subject: [PATCH 2/5] factor out BARF_UNLESS_COPYABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- 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) From 08e8c266653a486cc441ec031136875bf579f054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 1 Jan 2023 22:11:20 +0100 Subject: [PATCH 3/5] do full type check in BARF_UNLESS_COPYABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- 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) From d2ec87a684e2f9cd1f0c653620a00d74ad5ee2ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 1 Jan 2023 22:14:12 +0100 Subject: [PATCH 4/5] add DUP_ARRAY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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: From 6e578410960d9ceb35ec98ad4b6fc711f1a9c85c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 1 Jan 2023 22:16:48 +0100 Subject: [PATCH 5/5] use DUP_ARRAY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 e131eb9b07..cd8195ab11 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);