* [PATCH 1/2] add MOVE_ARRAY @ 2017-07-15 19:36 René Scharfe 2017-07-15 20:00 ` [PATCH 2/2] use MOVE_ARRAY René Scharfe ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: René Scharfe @ 2017-07-15 19:36 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Jeff King 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 <l.s.r@web.de> --- 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 @@ -27,6 +27,23 @@ expression 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; expression n; @@ 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: -- 2.13.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] use MOVE_ARRAY 2017-07-15 19:36 [PATCH 1/2] add MOVE_ARRAY René Scharfe @ 2017-07-15 20:00 ` René Scharfe 2017-07-15 20:20 ` [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() René Scharfe 2017-07-16 10:31 ` [PATCH 1/2] add MOVE_ARRAY Jeff King 2 siblings, 0 replies; 7+ messages in thread From: René Scharfe @ 2017-07-15 20:00 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Jeff King 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 <l.s.r@web.de> --- Downside: The one in builtin/ls-files.c papers over a report by UBSan for the case of istate->cache being NULL. Technically we still try to add pos to NULL if pruning an empty index, but we won't see it anymore. Will send a separate patch. 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); } } -- 2.13.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() 2017-07-15 19:36 [PATCH 1/2] add MOVE_ARRAY René Scharfe 2017-07-15 20:00 ` [PATCH 2/2] use MOVE_ARRAY René Scharfe @ 2017-07-15 20:20 ` René Scharfe 2017-07-16 0:31 ` Ramsay Jones 2017-07-16 10:31 ` [PATCH 1/2] add MOVE_ARRAY Jeff King 2 siblings, 1 reply; 7+ messages in thread From: René Scharfe @ 2017-07-15 20:20 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Jeff King 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 <l.s.r@web.de> --- I don't know why the rules in contrib/coccinelle/array.cocci didn't match. :-? 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; -- 2.13.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() 2017-07-15 20:20 ` [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() René Scharfe @ 2017-07-16 0:31 ` Ramsay Jones 2017-07-16 4:04 ` René Scharfe 0 siblings, 1 reply; 7+ messages in thread From: Ramsay Jones @ 2017-07-16 0:31 UTC (permalink / raw) To: René Scharfe, Git List; +Cc: Junio C Hamano, Jeff King On 15/07/17 21:20, René Scharfe wrote: > 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 <l.s.r@web.de> > --- > I don't know why the rules in contrib/coccinelle/array.cocci didn't > match. :-? > > 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); My patch looks like: - memcpy(img->line + applied_pos, - postimage->line, - postimage->nr * sizeof(*img->line)); + if (postimage->line && postimage->nr) + memcpy(img->line + applied_pos, + postimage->line, + postimage->nr * sizeof(*img->line)); ... which I think I prefer (slightly). ATB, Ramsay Jones > if (!state->allow_overlap) > for (i = 0; i < postimage->nr; i++) > img->line[applied_pos + i].flag |= LINE_PATCHED; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() 2017-07-16 0:31 ` Ramsay Jones @ 2017-07-16 4:04 ` René Scharfe 0 siblings, 0 replies; 7+ messages in thread From: René Scharfe @ 2017-07-16 4:04 UTC (permalink / raw) To: Ramsay Jones, Git List; +Cc: Junio C Hamano, Jeff King Am 16.07.2017 um 02:31 schrieb Ramsay Jones: > > > On 15/07/17 21:20, René Scharfe wrote: >> 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 <l.s.r@web.de> >> --- >> I don't know why the rules in contrib/coccinelle/array.cocci didn't >> match. :-? >> >> 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); > > My patch looks like: > > - memcpy(img->line + applied_pos, > - postimage->line, > - postimage->nr * sizeof(*img->line)); > + if (postimage->line && postimage->nr) > + memcpy(img->line + applied_pos, > + postimage->line, > + postimage->nr * sizeof(*img->line)); > > ... which I think I prefer (slightly). Can postimage->line be NULL when postimage->nr is bigger than 0? What would that mean? The only ways to arrive at that point that I an come up with are bugs (we accidentally set ->line to NULL, or we forgot to clean ->line). We'd better notice them early by getting a nice shrieking segfault. Adding an assert would work as well. René ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] add MOVE_ARRAY 2017-07-15 19:36 [PATCH 1/2] add MOVE_ARRAY René Scharfe 2017-07-15 20:00 ` [PATCH 2/2] use MOVE_ARRAY René Scharfe 2017-07-15 20:20 ` [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() René Scharfe @ 2017-07-16 10:31 ` Jeff King 2 siblings, 0 replies; 7+ messages in thread From: Jeff King @ 2017-07-16 10:31 UTC (permalink / raw) To: René Scharfe; +Cc: Git List, Junio C Hamano On Sat, Jul 15, 2017 at 09:36:20PM +0200, René Scharfe wrote: > 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. If we have COPY_ARRAY(), I think it's foolish not to provide the matching MOVE_ARRAY(). If it were just the "if (n)", I might say we could just do this inline in the few calls that care. But I really like the size safety. I agree with your comments elsewhere that we don't need to worry about the case where one of the arrays is NULL but the size is not zero. That's a flat-out bug. -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. @ 2017-04-06 8:02 Martin Liška 2017-04-06 8:34 ` Jeff King 0 siblings, 1 reply; 7+ messages in thread From: Martin Liška @ 2017-04-06 8:02 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 140 bytes --] Hello. Following patch fixes issues that can be seen with -fsanitize=undefined on GCC 7. Patch was tested with make test. Thanks, Martin [-- Attachment #2: 0001-Fix-nonnull-errors-reported-by-UBSAN-with-GCC-7.patch --] [-- Type: text/x-patch, Size: 1596 bytes --] From e6d2d5ee5614acdbe67b79aeb0fdc9b53cf3a828 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 5 Apr 2017 14:31:32 +0200 Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. Memory functions like memmove and memcpy should not be called with an argument equal to NULL. Signed-off-by: Martin Liska <mliska@suse.cz> --- apply.c | 7 ++++--- builtin/ls-files.c | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/apply.c b/apply.c index e6dbab26a..78a83f66b 100644 --- a/apply.c +++ b/apply.c @@ -2802,9 +2802,10 @@ static void update_image(struct apply_state *state, 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)); + if (postimage->nr) + memcpy(img->line + applied_pos, + postimage->line, + postimage->nr * sizeof(*img->line)); if (!state->allow_overlap) for (i = 0; i < postimage->nr; i++) img->line[applied_pos + i].flag |= LINE_PATCHED; diff --git a/builtin/ls-files.c b/builtin/ls-files.c index d449e46db..01d24314d 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -391,8 +391,9 @@ static void prune_cache(const char *prefix, size_t prefixlen) } last = next; } - memmove(active_cache, active_cache + pos, - (last - pos) * sizeof(struct cache_entry *)); + if (last - pos > 0) + memmove(active_cache, active_cache + pos, + (last - pos) * sizeof(struct cache_entry *)); active_nr = last - pos; } -- 2.12.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 8:02 [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7 Martin Liška @ 2017-04-06 8:34 ` Jeff King 2017-04-06 9:52 ` [PATCH v2 " Martin Liška 0 siblings, 1 reply; 7+ messages in thread From: Jeff King @ 2017-04-06 8:34 UTC (permalink / raw) To: Martin Liška; +Cc: git On Thu, Apr 06, 2017 at 10:02:22AM +0200, Martin Liška wrote: > Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. > > Memory functions like memmove and memcpy should not be called > with an argument equal to NULL. Yeah, makes sense. Your fixes are obviously correct. In other cases we've added wrappers like sane_qsort() that do the size check automatically. I'm not sure if we'd want to do the same here. Either way, it probably makes sense to take this as a quick fix and worry about refactoring as a possible patch on top. Thanks. -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 8:34 ` Jeff King @ 2017-04-06 9:52 ` Martin Liška 2017-04-06 12:26 ` René Scharfe 0 siblings, 1 reply; 7+ messages in thread From: Martin Liška @ 2017-04-06 9:52 UTC (permalink / raw) To: Jeff King; +Cc: git [-- Attachment #1: Type: text/plain, Size: 841 bytes --] On 04/06/2017 10:34 AM, Jeff King wrote: > On Thu, Apr 06, 2017 at 10:02:22AM +0200, Martin Liška wrote: > >> Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. >> >> Memory functions like memmove and memcpy should not be called >> with an argument equal to NULL. > > Yeah, makes sense. Your fixes are obviously correct. In other cases > we've added wrappers like sane_qsort() that do the size check > automatically. I'm not sure if we'd want to do the same here. > > Either way, it probably makes sense to take this as a quick fix and > worry about refactoring as a possible patch on top. > > Thanks. > > -Peff > Hello. I'm sending (v2), where I updated commit message and wrapped 2 problematic places to newly introduced macros that do the check. Follow-up patch can change usages of memcpy and memove. Martin [-- Attachment #2: 0001-Fix-nonnull-errors-reported-by-UBSAN-with-GCC-7.patch --] [-- Type: text/x-patch, Size: 2331 bytes --] From 876cfa4f4b2e74d43b9fd93d1056b88ab2ace0cd Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 5 Apr 2017 14:31:32 +0200 Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. Memory functions like memmove and memcpy should not be called with an argument equal to NULL. Signed-off-by: Martin Liska <mliska@suse.cz> --- apply.c | 6 +++--- builtin/ls-files.c | 2 +- git-compat-util.h | 20 ++++++++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/apply.c b/apply.c index e6dbab26a..eacca29fa 100644 --- a/apply.c +++ b/apply.c @@ -2802,9 +2802,9 @@ static void update_image(struct apply_state *state, 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)); + MEMCPY(img->line + applied_pos, + postimage->line, + postimage->nr * sizeof(*img->line)); if (!state->allow_overlap) for (i = 0; i < postimage->nr; i++) img->line[applied_pos + i].flag |= LINE_PATCHED; diff --git a/builtin/ls-files.c b/builtin/ls-files.c index d449e46db..7caeeb6a6 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen) } last = next; } - memmove(active_cache, active_cache + pos, + MEMMOVE(active_cache, active_cache + pos, (last - pos) * sizeof(struct cache_entry *)); active_nr = last - pos; } diff --git a/git-compat-util.h b/git-compat-util.h index 8a4a3f85e..e5323f6c7 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1002,6 +1002,26 @@ int git_qsort_s(void *base, size_t nmemb, size_t size, die("BUG: qsort_s() failed"); \ } while (0) +static inline void *sane_memcpy(void *dest, const void *src, size_t n) +{ + if (n > 0) + return memcpy(dest, src, n); + else + return dest; +} + +#define MEMCPY(dest, src, n) sane_memcpy(dest, src, n) + +static inline void *sane_memmove(void *dest, const void *src, size_t n) +{ + if (n > 0) + return memmove(dest, src, n); + else + return dest; +} + +#define MEMMOVE(dest, src, n) sane_memmove(dest, src, n) + #ifndef REG_STARTEND #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd" #endif -- 2.12.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 9:52 ` [PATCH v2 " Martin Liška @ 2017-04-06 12:26 ` René Scharfe 2017-04-06 15:42 ` [PATCH v3 " Martin Liška 0 siblings, 1 reply; 7+ messages in thread From: René Scharfe @ 2017-04-06 12:26 UTC (permalink / raw) To: Martin Liška, Jeff King; +Cc: git Am 06.04.2017 um 11:52 schrieb Martin Liška: > I'm sending (v2), where I updated commit message and wrapped 2 problematic > places to newly introduced macros that do the check. Follow-up patch can > change usages of memcpy and memove. > diff --git a/apply.c b/apply.c > index e6dbab26a..eacca29fa 100644 > --- a/apply.c > +++ b/apply.c > @@ -2802,9 +2802,9 @@ static void update_image(struct apply_state *state, > 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)); > + MEMCPY(img->line + applied_pos, > + postimage->line, > + postimage->nr * sizeof(*img->line)); Using the existing macro COPY_ARRAY would yield a nicer result: 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; > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > index d449e46db..7caeeb6a6 100644 > --- a/builtin/ls-files.c > +++ b/builtin/ls-files.c > @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen) > } > last = next; > } > - memmove(active_cache, active_cache + pos, > + MEMMOVE(active_cache, active_cache + pos, > (last - pos) * sizeof(struct cache_entry *)); > active_nr = last - pos; > } Perhaps we should add MOVE_ARRAY to complement COPY_ARRAY. René ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 12:26 ` René Scharfe @ 2017-04-06 15:42 ` Martin Liška 2017-04-06 16:33 ` Johannes Sixt 0 siblings, 1 reply; 7+ messages in thread From: Martin Liška @ 2017-04-06 15:42 UTC (permalink / raw) To: René Scharfe, Jeff King; +Cc: git [-- Attachment #1: Type: text/plain, Size: 1680 bytes --] On 04/06/2017 02:26 PM, René Scharfe wrote: > Am 06.04.2017 um 11:52 schrieb Martin Liška: >> I'm sending (v2), where I updated commit message and wrapped 2 problematic >> places to newly introduced macros that do the check. Follow-up patch can >> change usages of memcpy and memove. > >> diff --git a/apply.c b/apply.c >> index e6dbab26a..eacca29fa 100644 >> --- a/apply.c >> +++ b/apply.c >> @@ -2802,9 +2802,9 @@ static void update_image(struct apply_state *state, >> 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)); >> + MEMCPY(img->line + applied_pos, >> + postimage->line, >> + postimage->nr * sizeof(*img->line)); > > Using the existing macro COPY_ARRAY would yield a nicer result: Yes, it's nicer. I'm sending tested version 3. Martin > > 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; >> diff --git a/builtin/ls-files.c b/builtin/ls-files.c >> index d449e46db..7caeeb6a6 100644 >> --- a/builtin/ls-files.c >> +++ b/builtin/ls-files.c >> @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen) >> } >> last = next; >> } >> - memmove(active_cache, active_cache + pos, >> + MEMMOVE(active_cache, active_cache + pos, >> (last - pos) * sizeof(struct cache_entry *)); >> active_nr = last - pos; >> } > > Perhaps we should add MOVE_ARRAY to complement COPY_ARRAY. > > René > [-- Attachment #2: 0001-Fix-nonnull-errors-reported-by-UBSAN-with-GCC-7.patch --] [-- Type: text/x-patch, Size: 2156 bytes --] From 4784ff90b2c4cd0d78a25c8d8ed77f299686348c Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 5 Apr 2017 14:31:32 +0200 Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. Memory functions like memmove and memcpy should only be called with positive sizes. That is achieved by newly introduced macro MEMMOVE and usage of ARRAY_COPY. Signed-off-by: Martin Liska <mliska@suse.cz> --- apply.c | 4 +--- builtin/ls-files.c | 2 +- git-compat-util.h | 10 ++++++++++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/apply.c b/apply.c index e6dbab26a..121f3f414 100644 --- a/apply.c +++ b/apply.c @@ -2802,9 +2802,7 @@ static void update_image(struct apply_state *state, 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)); + 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; diff --git a/builtin/ls-files.c b/builtin/ls-files.c index d449e46db..7caeeb6a6 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen) } last = next; } - memmove(active_cache, active_cache + pos, + MEMMOVE(active_cache, active_cache + pos, (last - pos) * sizeof(struct cache_entry *)); active_nr = last - pos; } diff --git a/git-compat-util.h b/git-compat-util.h index 8a4a3f85e..b75e21cff 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1002,6 +1002,16 @@ int git_qsort_s(void *base, size_t nmemb, size_t size, die("BUG: qsort_s() failed"); \ } while (0) +static inline void *sane_memmove(void *dest, const void *src, size_t n) +{ + if (n > 0) + return memmove(dest, src, n); + else + return dest; +} + +#define MEMMOVE(dest, src, n) sane_memmove(dest, src, n) + #ifndef REG_STARTEND #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd" #endif -- 2.12.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 15:42 ` [PATCH v3 " Martin Liška @ 2017-04-06 16:33 ` Johannes Sixt 2017-04-06 17:31 ` René Scharfe 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2017-04-06 16:33 UTC (permalink / raw) To: Martin Liška; +Cc: René Scharfe, Jeff King, git Am 06.04.2017 um 17:42 schrieb Martin Liška: > +static inline void *sane_memmove(void *dest, const void *src, size_t n) > +{ > + if (n > 0) > + return memmove(dest, src, n); > + else > + return dest; > +} Huh? memmove with n == 0 is well-defined. This wrapper is pointless. -- Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 16:33 ` Johannes Sixt @ 2017-04-06 17:31 ` René Scharfe 2017-04-06 20:49 ` Johannes Sixt 0 siblings, 1 reply; 7+ messages in thread From: René Scharfe @ 2017-04-06 17:31 UTC (permalink / raw) To: Johannes Sixt, Martin Liška; +Cc: Jeff King, git Am 06.04.2017 um 18:33 schrieb Johannes Sixt: > Am 06.04.2017 um 17:42 schrieb Martin Liška: >> +static inline void *sane_memmove(void *dest, const void *src, size_t n) >> +{ >> + if (n > 0) >> + return memmove(dest, src, n); >> + else >> + return dest; >> +} > > Huh? memmove with n == 0 is well-defined. This wrapper is pointless. memmove(3) with NULL pointers is undefined. From string.h on Debian: extern void *memmove (void *__dest, const void *__src, size_t __n) __THROW __nonnull ((1, 2)); Sometimes we use a NULL pointer and a size of zero to represent arrays with no members. That convention is incompatible with memmove(3), but the wrapper above would support it. Checking the size instead of the pointer is preferable because a positive length with NULL pointers should still result in a segfault instead of a silent no-op. (I'd still prefer a MOVE_ARRAY macro which also infers the element size). René ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 17:31 ` René Scharfe @ 2017-04-06 20:49 ` Johannes Sixt 2017-04-07 14:23 ` Martin Liška 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2017-04-06 20:49 UTC (permalink / raw) To: René Scharfe; +Cc: Martin Liška, Jeff King, git Am 06.04.2017 um 19:31 schrieb René Scharfe: > Am 06.04.2017 um 18:33 schrieb Johannes Sixt: >> Am 06.04.2017 um 17:42 schrieb Martin Liška: >>> +static inline void *sane_memmove(void *dest, const void *src, size_t n) >>> +{ >>> + if (n > 0) >>> + return memmove(dest, src, n); >>> + else >>> + return dest; >>> +} >> >> Huh? memmove with n == 0 is well-defined. This wrapper is pointless. > > memmove(3) with NULL pointers is undefined. Then don't hide this helper behind a macro with a suspiciously similar name. Using the name sane_mmemove at the call site is preferable. memmove_or_null or something similar perhaps even more so. -- Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] Fix nonnull errors reported by UBSAN with GCC 7. 2017-04-06 20:49 ` Johannes Sixt @ 2017-04-07 14:23 ` Martin Liška 2017-04-07 15:25 ` [PATCH 2/2] use MOVE_ARRAY René Scharfe 0 siblings, 1 reply; 7+ messages in thread From: Martin Liška @ 2017-04-07 14:23 UTC (permalink / raw) To: Johannes Sixt, René Scharfe; +Cc: Jeff King, git [-- Attachment #1: Type: text/plain, Size: 773 bytes --] On 04/06/2017 10:49 PM, Johannes Sixt wrote: > Am 06.04.2017 um 19:31 schrieb René Scharfe: >> Am 06.04.2017 um 18:33 schrieb Johannes Sixt: >>> Am 06.04.2017 um 17:42 schrieb Martin Liška: >>>> +static inline void *sane_memmove(void *dest, const void *src, size_t n) >>>> +{ >>>> + if (n > 0) >>>> + return memmove(dest, src, n); >>>> + else >>>> + return dest; >>>> +} >>> >>> Huh? memmove with n == 0 is well-defined. This wrapper is pointless. >> >> memmove(3) with NULL pointers is undefined. > > Then don't hide this helper behind a macro with a suspiciously similar name. Using the name sane_mmemove at the call site is preferable. memmove_or_null or something similar perhaps even more so. > > -- Hannes > Good. There's tested v4. Martin [-- Attachment #2: 0001-Fix-nonnull-errors-reported-by-UBSAN-with-GCC-7.patch --] [-- Type: text/x-patch, Size: 2055 bytes --] From 0bdf4d717d3d368dd9676d15d20f8592c4d22fde Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Wed, 5 Apr 2017 14:31:32 +0200 Subject: [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7. Replace call to memmove with newly introduced function memmove_or_null and call to memcpy with COPY_ARRAY macro. Signed-off-by: Martin Liska <mliska@suse.cz> --- apply.c | 4 +--- builtin/ls-files.c | 2 +- git-compat-util.h | 8 ++++++++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/apply.c b/apply.c index e6dbab26a..121f3f414 100644 --- a/apply.c +++ b/apply.c @@ -2802,9 +2802,7 @@ static void update_image(struct apply_state *state, 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)); + 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; diff --git a/builtin/ls-files.c b/builtin/ls-files.c index d449e46db..0a6cc1e8a 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -391,7 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen) } last = next; } - memmove(active_cache, active_cache + pos, + memmove_or_null(active_cache, active_cache + pos, (last - pos) * sizeof(struct cache_entry *)); active_nr = last - pos; } diff --git a/git-compat-util.h b/git-compat-util.h index 8a4a3f85e..81f6e56ac 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1002,6 +1002,14 @@ int git_qsort_s(void *base, size_t nmemb, size_t size, die("BUG: qsort_s() failed"); \ } while (0) +static inline void *memmove_or_null(void *dest, const void *src, size_t n) +{ + if (n > 0) + return memmove(dest, src, n); + else + return dest; +} + #ifndef REG_STARTEND #error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd" #endif -- 2.12.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] use MOVE_ARRAY 2017-04-07 14:23 ` Martin Liška @ 2017-04-07 15:25 ` René Scharfe 0 siblings, 0 replies; 7+ messages in thread From: René Scharfe @ 2017-04-07 15:25 UTC (permalink / raw) To: Martin Liška, git, Junio C Hamano; +Cc: Johannes Sixt, Jeff King Add a semantic patch for converting certain calls of memmove(3) to MOVE_ARRAY() and apply that transformation to the code base. The result is shorter and safer code. Signed-off-by: Rene Scharfe <l.s.r@web.de> --- apply.c | 7 +++---- builtin/ls-files.c | 3 +-- builtin/merge.c | 2 +- builtin/pack-objects.c | 5 ++--- cache-tree.c | 5 ++--- commit.c | 11 ++++------- contrib/coccinelle/array.cocci | 37 +++++++++++++++++++++++++++++++++++++ diffcore-rename.c | 8 ++++---- dir.c | 4 ++-- notes-merge.c | 3 +-- parse-options.c | 2 +- read-cache.c | 5 ++--- reflog-walk.c | 7 +++---- refs/files-backend.c | 5 ++--- replace_object.c | 6 ++---- rerere.c | 4 ++-- string-list.c | 5 ++--- 17 files changed, 71 insertions(+), 48 deletions(-) diff --git a/apply.c b/apply.c index e6dbab26ad..4e4d13cacc 100644 --- a/apply.c +++ b/apply.c @@ -2798,10 +2798,9 @@ 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)); + MOVE_ARRAY(img->line + applied_pos + postimage->nr, + img->line + applied_pos + preimage_limit, + img->nr - (applied_pos + preimage_limit)); memcpy(img->line + applied_pos, postimage->line, postimage->nr * sizeof(*img->line)); diff --git a/builtin/ls-files.c b/builtin/ls-files.c index d449e46db5..1627c26466 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -391,8 +391,7 @@ static void prune_cache(const char *prefix, size_t prefixlen) } last = next; } - memmove(active_cache, active_cache + pos, - (last - pos) * sizeof(struct cache_entry *)); + MOVE_ARRAY(active_cache, active_cache + pos, last - pos); active_nr = last - pos; } diff --git a/builtin/merge.c b/builtin/merge.c index 95572b1810..0c0d2e4a94 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -536,7 +536,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 84af7c2324..5fb5d7c546 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1292,9 +1292,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 345ea35963..071164a2ec 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 73c78c2b80..62a71fee92 100644 --- a/commit.c +++ b/commit.c @@ -125,10 +125,8 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups) ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc); commit_graft_nr++; if (pos < commit_graft_nr) - memmove(commit_graft + pos + 1, - commit_graft + pos, - (commit_graft_nr - pos - 1) * - sizeof(*commit_graft)); + MOVE_ARRAY(commit_graft + pos + 1, commit_graft + pos, + commit_graft_nr - pos - 1); commit_graft[pos] = graft; return 0; } @@ -222,9 +220,8 @@ int unregister_shallow(const unsigned char *sha1) 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/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci index 4ba98b7eaf..a6e5ee927f 100644 --- a/contrib/coccinelle/array.cocci +++ b/contrib/coccinelle/array.cocci @@ -27,6 +27,43 @@ expression n; @@ type T; +T *dst; +T *src; +expression n; +@@ +( +- memmove( ++ MOVE_ARRAY( +( + dst +| + dst + ... +| + &dst[...] +) +- , src, (n) * sizeof(*dst) ++ , src, n + ); +| +- memmove(dst, ++ MOVE_ARRAY(dst, +( + src +| + src + ... +| + &src[...] +) +- , (n) * sizeof(*src) ++ , n + ); +| +- memmove(dst, src, (n) * sizeof(T)); ++ MOVE_ARRAY(dst, src, n); +) + +@@ +type T; T *ptr; expression n; @@ diff --git a/diffcore-rename.c b/diffcore-rename.c index f7444c86bd..449d5f065f 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -57,8 +57,8 @@ static int add_rename_dst(struct diff_filespec *two) ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc); rename_dst_nr++; if (first < rename_dst_nr) - memmove(rename_dst + first + 1, rename_dst + first, - (rename_dst_nr - first - 1) * sizeof(*rename_dst)); + MOVE_ARRAY(rename_dst + first + 1, rename_dst + first, + rename_dst_nr - first - 1); rename_dst[first].two = alloc_filespec(two->path); fill_filespec(rename_dst[first].two, two->oid.hash, two->oid_valid, two->mode); @@ -98,8 +98,8 @@ static struct diff_rename_src *register_rename_src(struct diff_filepair *p) ALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc); rename_src_nr++; if (first < rename_src_nr) - memmove(rename_src + first + 1, rename_src + first, - (rename_src_nr - first - 1) * sizeof(*rename_src)); + MOVE_ARRAY(rename_src + first + 1, rename_src + first, + rename_src_nr - first - 1); rename_src[first].p = p; rename_src[first].score = score; return &(rename_src[first]); diff --git a/dir.c b/dir.c index f451bfa48c..f5f5658b1d 100644 --- a/dir.c +++ b/dir.c @@ -691,8 +691,8 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc, FLEX_ALLOC_MEM(d, name, name, len); ALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc); - memmove(dir->dirs + first + 1, dir->dirs + first, - (dir->dirs_nr - first) * sizeof(*dir->dirs)); + MOVE_ARRAY(dir->dirs + first + 1, dir->dirs + first, + dir->dirs_nr - first); dir->dirs_nr++; dir->dirs[first] = d; return d; diff --git a/notes-merge.c b/notes-merge.c index 5998605acc..36d5d2f87f 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/parse-options.c b/parse-options.c index a23a1e67f0..b8f768b0d8 100644 --- a/parse-options.c +++ b/parse-options.c @@ -524,7 +524,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, int parse_options_end(struct parse_opt_ctx_t *ctx) { - memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out)); + MOVE_ARRAY(ctx->out + ctx->cpidx, ctx->argv, ctx->argc); ctx->out[ctx->cpidx + ctx->argc] = NULL; return ctx->cpidx + ctx->argc; } diff --git a/read-cache.c b/read-cache.c index e447751823..1bb5e97979 100644 --- a/read-cache.c +++ b/read-cache.c @@ -514,9 +514,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 99679f5825..25c198b9fd 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -95,10 +95,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/refs/files-backend.c b/refs/files-backend.c index 50188e92f9..a851326445 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -437,9 +437,8 @@ static int remove_entry(struct ref_dir *dir, const char *refname) return -1; entry = dir->entries[entry_index]; - memmove(&dir->entries[entry_index], - &dir->entries[entry_index + 1], - (dir->nr - entry_index - 1) * sizeof(*dir->entries) + MOVE_ARRAY(&dir->entries[entry_index], &dir->entries[entry_index + 1], + dir->nr - entry_index - 1 ); dir->nr--; if (dir->sorted > entry_index) diff --git a/replace_object.c b/replace_object.c index f0b39f06d5..3e49965d05 100644 --- a/replace_object.c +++ b/replace_object.c @@ -44,10 +44,8 @@ static int register_replace_object(struct replace_object *replace, ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc); replace_object_nr++; if (pos < replace_object_nr) - memmove(replace_object + pos + 1, - replace_object + pos, - (replace_object_nr - pos - 1) * - sizeof(*replace_object)); + MOVE_ARRAY(replace_object + pos + 1, replace_object + pos, + replace_object_nr - pos - 1); replace_object[pos] = replace; return 0; } diff --git a/rerere.c b/rerere.c index 3bd55caf3b..7410445ba2 100644 --- a/rerere.c +++ b/rerere.c @@ -159,8 +159,8 @@ static struct rerere_dir *find_rerere_dir(const char *hex) ALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc); /* ... and add it in. */ rerere_dir_nr++; - memmove(rerere_dir + pos + 1, rerere_dir + pos, - (rerere_dir_nr - pos - 1) * sizeof(*rerere_dir)); + MOVE_ARRAY(rerere_dir + pos + 1, rerere_dir + pos, + rerere_dir_nr - pos - 1); rerere_dir[pos] = rr_dir; scan_rerere_dir(rr_dir); } diff --git a/string-list.c b/string-list.c index 45016ad86d..df64d4c446 100644 --- a/string-list.c +++ b/string-list.c @@ -46,9 +46,8 @@ static int add_entry(int insert_at, struct string_list *list, const char *string REALLOC_ARRAY(list->items, 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; -- 2.12.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-16 10:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-15 19:36 [PATCH 1/2] add MOVE_ARRAY René Scharfe 2017-07-15 20:00 ` [PATCH 2/2] use MOVE_ARRAY René Scharfe 2017-07-15 20:20 ` [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image() René Scharfe 2017-07-16 0:31 ` Ramsay Jones 2017-07-16 4:04 ` René Scharfe 2017-07-16 10:31 ` [PATCH 1/2] add MOVE_ARRAY Jeff King -- strict thread matches above, loose matches on Subject: below -- 2017-04-06 8:02 [PATCH 1/2] Fix nonnull errors reported by UBSAN with GCC 7 Martin Liška 2017-04-06 8:34 ` Jeff King 2017-04-06 9:52 ` [PATCH v2 " Martin Liška 2017-04-06 12:26 ` René Scharfe 2017-04-06 15:42 ` [PATCH v3 " Martin Liška 2017-04-06 16:33 ` Johannes Sixt 2017-04-06 17:31 ` René Scharfe 2017-04-06 20:49 ` Johannes Sixt 2017-04-07 14:23 ` Martin Liška 2017-04-07 15:25 ` [PATCH 2/2] use MOVE_ARRAY René Scharfe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).