From 578398071e45d296c3dc1de10acdbc15365e763f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 15 Jul 2017 21:36:20 +0200 Subject: [PATCH 1/4] add MOVE_ARRAY Similar to COPY_ARRAY (introduced in 60566cbb58), add a safe and convenient helper for moving potentially overlapping ranges of array entries. It infers the element size, multiplies automatically and safely to get the size in bytes, does a basic type safety check by comparing element sizes and unlike memmove(3) it supports NULL pointers iff 0 elements are to be moved. Also add a semantic patch to demonstrate the helper's intended usage. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- contrib/coccinelle/array.cocci | 17 +++++++++++++++++ git-compat-util.h | 8 ++++++++ 2 files changed, 25 insertions(+) diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci index 4ba98b7eaf..c61d1ca8dc 100644 --- a/contrib/coccinelle/array.cocci +++ b/contrib/coccinelle/array.cocci @@ -25,6 +25,23 @@ expression n; - memcpy(dst, src, n * sizeof(T)); + COPY_ARRAY(dst, src, n); +@@ +type T; +T *dst; +T *src; +expression n; +@@ +( +- memmove(dst, src, (n) * sizeof(*dst)); ++ MOVE_ARRAY(dst, src, n); +| +- memmove(dst, src, (n) * sizeof(*src)); ++ MOVE_ARRAY(dst, src, n); +| +- memmove(dst, src, (n) * sizeof(T)); ++ MOVE_ARRAY(dst, src, n); +) + @@ type T; T *ptr; diff --git a/git-compat-util.h b/git-compat-util.h index 047172d173..159f82154a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -825,6 +825,14 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size) memcpy(dst, src, st_mult(size, n)); } +#define MOVE_ARRAY(dst, src, n) move_array((dst), (src), (n), sizeof(*(dst)) + \ + BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src)))) +static inline void move_array(void *dst, const void *src, size_t n, size_t size) +{ + if (n) + memmove(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: From f331ab9d4cb21942dcde6d879aaca6a1784e8cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 15 Jul 2017 22:00:45 +0200 Subject: [PATCH 2/4] use MOVE_ARRAY Simplify the code for moving members inside of an array and make it more robust by using the helper macro MOVE_ARRAY. It calculates the size based on the specified number of elements for us and supports NULL pointers when that number is zero. Raw memmove(3) calls with NULL can cause the compiler to (over-eagerly) optimize out later NULL checks. This patch was generated with contrib/coccinelle/array.cocci and spatch (Coccinelle). Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/ls-files.c | 3 +-- builtin/merge.c | 2 +- builtin/pack-objects.c | 5 ++--- cache-tree.c | 5 ++--- commit.c | 5 ++--- notes-merge.c | 3 +-- read-cache.c | 5 ++--- reflog-walk.c | 7 +++---- string-list.c | 8 +++----- 9 files changed, 17 insertions(+), 26 deletions(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index b8514a0029..dc4a6aa3d9 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -378,8 +378,7 @@ static void prune_index(struct index_state *istate, } last = next; } - memmove(istate->cache, istate->cache + pos, - (last - pos) * sizeof(struct cache_entry *)); + MOVE_ARRAY(istate->cache, istate->cache + pos, last - pos); istate->cache_nr = last - pos; } diff --git a/builtin/merge.c b/builtin/merge.c index 900bafdb45..d5797b8fe7 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -537,7 +537,7 @@ static void parse_branch_merge_options(char *bmo) die(_("Bad branch.%s.mergeoptions string: %s"), branch, split_cmdline_strerror(argc)); REALLOC_ARRAY(argv, argc + 2); - memmove(argv + 1, argv, sizeof(*argv) * (argc + 1)); + MOVE_ARRAY(argv + 1, argv, argc + 1); argc++; argv[0] = "branch.*.mergeoptions"; parse_options(argc, argv, NULL, builtin_merge_options, diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index f4a8441fe9..e730b415bf 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1298,9 +1298,8 @@ static int check_pbase_path(unsigned hash) done_pbase_paths_alloc); done_pbase_paths_num++; if (pos < done_pbase_paths_num) - memmove(done_pbase_paths + pos + 1, - done_pbase_paths + pos, - (done_pbase_paths_num - pos - 1) * sizeof(unsigned)); + MOVE_ARRAY(done_pbase_paths + pos + 1, done_pbase_paths + pos, + done_pbase_paths_num - pos - 1); done_pbase_paths[pos] = hash; return 0; } diff --git a/cache-tree.c b/cache-tree.c index ec23d8c03d..2440d1dc89 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -131,9 +131,8 @@ static int do_invalidate_path(struct cache_tree *it, const char *path) * move 4 and 5 up one place (2 entries) * 2 = 6 - 3 - 1 = subtree_nr - pos - 1 */ - memmove(it->down+pos, it->down+pos+1, - sizeof(struct cache_tree_sub *) * - (it->subtree_nr - pos - 1)); + MOVE_ARRAY(it->down + pos, it->down + pos + 1, + it->subtree_nr - pos - 1); it->subtree_nr--; } return 1; diff --git a/commit.c b/commit.c index cbfd689939..d3150d6270 100644 --- a/commit.c +++ b/commit.c @@ -223,9 +223,8 @@ int unregister_shallow(const struct object_id *oid) if (pos < 0) return -1; if (pos + 1 < commit_graft_nr) - memmove(commit_graft + pos, commit_graft + pos + 1, - sizeof(struct commit_graft *) - * (commit_graft_nr - pos - 1)); + MOVE_ARRAY(commit_graft + pos, commit_graft + pos + 1, + commit_graft_nr - pos - 1); commit_graft_nr--; return 0; } diff --git a/notes-merge.c b/notes-merge.c index 70e3fbeefb..c12b354f10 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -99,8 +99,7 @@ static struct notes_merge_pair *find_notes_merge_pair_pos( else { *occupied = 0; if (insert_new && i < len) { - memmove(list + i + 1, list + i, - (len - i) * sizeof(struct notes_merge_pair)); + MOVE_ARRAY(list + i + 1, list + i, len - i); memset(list + i, 0, sizeof(struct notes_merge_pair)); } } diff --git a/read-cache.c b/read-cache.c index 2121b6e7bb..acfb028f48 100644 --- a/read-cache.c +++ b/read-cache.c @@ -515,9 +515,8 @@ int remove_index_entry_at(struct index_state *istate, int pos) istate->cache_nr--; if (pos >= istate->cache_nr) return 0; - memmove(istate->cache + pos, - istate->cache + pos + 1, - (istate->cache_nr - pos) * sizeof(struct cache_entry *)); + MOVE_ARRAY(istate->cache + pos, istate->cache + pos + 1, + istate->cache_nr - pos); return 1; } diff --git a/reflog-walk.c b/reflog-walk.c index 081f89b70d..81bca2ed23 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -111,10 +111,9 @@ static struct commit_info *get_commit_info(struct commit *commit, struct commit_info *result = &lifo->items[i]; if (pop) { if (i + 1 < lifo->nr) - memmove(lifo->items + i, - lifo->items + i + 1, - (lifo->nr - i) * - sizeof(struct commit_info)); + MOVE_ARRAY(lifo->items + i, + lifo->items + i + 1, + lifo->nr - i); lifo->nr--; } return result; diff --git a/string-list.c b/string-list.c index c650500c6e..806b4c8723 100644 --- a/string-list.c +++ b/string-list.c @@ -43,9 +43,8 @@ static int add_entry(int insert_at, struct string_list *list, const char *string ALLOC_GROW(list->items, list->nr+1, list->alloc); if (index < list->nr) - memmove(list->items + index + 1, list->items + index, - (list->nr - index) - * sizeof(struct string_list_item)); + MOVE_ARRAY(list->items + index + 1, list->items + index, + list->nr - index); list->items[index].string = list->strdup_strings ? xstrdup(string) : (char *)string; list->items[index].util = NULL; @@ -77,8 +76,7 @@ void string_list_remove(struct string_list *list, const char *string, free(list->items[i].util); list->nr--; - memmove(list->items + i, list->items + i + 1, - (list->nr - i) * sizeof(struct string_list_item)); + MOVE_ARRAY(list->items + i, list->items + i + 1, list->nr - i); } } From 177366415b95fb72bc2c37d55a936ff101986285 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 15 Jul 2017 22:20:54 +0200 Subject: [PATCH 3/4] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() Simplify the code by using the helper macros COPY_ARRAY and MOVE_ARRAY, which also makes them more robust in the case we copy or move no lines, as they allow using NULL points in that case, while memcpy(3) and memmove(3) don't. Found with Clang's UBSan. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- apply.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/apply.c b/apply.c index f2d599141d..40707ca50c 100644 --- a/apply.c +++ b/apply.c @@ -2809,13 +2809,10 @@ static void update_image(struct apply_state *state, img->line_allocated = img->line; } if (preimage_limit != postimage->nr) - memmove(img->line + applied_pos + postimage->nr, - img->line + applied_pos + preimage_limit, - (img->nr - (applied_pos + preimage_limit)) * - sizeof(*img->line)); - memcpy(img->line + applied_pos, - postimage->line, - postimage->nr * sizeof(*img->line)); + MOVE_ARRAY(img->line + applied_pos + postimage->nr, + img->line + applied_pos + preimage_limit, + img->nr - (applied_pos + preimage_limit)); + COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr); if (!state->allow_overlap) for (i = 0; i < postimage->nr; i++) img->line[applied_pos + i].flag |= LINE_PATCHED; From 168e63554cbd965fee4d0092e02f8170eba7481f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 16 Jul 2017 13:16:01 +0200 Subject: [PATCH 4/4] ls-files: don't try to prune an empty index Exit early when asked to prune an index that contains no entries to begin with. This avoids pointer arithmetic on istate->cache, which is possibly NULL in that case. Found with Clang's UBSan. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index dc4a6aa3d9..c6126eae55 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate, int pos; unsigned int first, last; - if (!prefix) + if (!prefix || !istate->cache_nr) return; pos = index_name_pos(istate, prefix, prefixlen); if (pos < 0)