Re: [PATCH v1] Travis: also test on 32-bit Linux
- From
René Scharfe <l.s.r@web.de>
- Date
- Mar 10, 2017, 00:14 UTC
- Message-ID
- <c553da50-e5ca-d064-e75c-46e5a5042935@web.de>
- In-Reply-To
- <20170305113618.ko2jymle4n5f2b5l@sigill.intra.peff.net>
Am 05.03.2017 um 12:36 schrieb Jeff King:
Show 6 quoted lines
> I grepped for 'memcpy.*sizeof' and found one other case that's not a > bug, but is questionable. > > Of the "good" cases, I think most of them could be converted into > something more obviously-correct, which would make auditing easier. The > three main cases I saw were:
Show 11 quoted lines
> 2. Ones which just copy a single object, like: > > memcpy(&dst, &src, sizeof(dst)); > > Perhaps we should be using struct assignment like: > > dst = src; > > here. It's safer and it should give the compiler more room to > optimize. The only downside is that if you have pointers, it is > easy to write "dst = src" when you meant "*dst = *src".
Compilers can usually inline memcpy(3) calls, but assignments are shorter and more pleasing to the eye, and we get a type check for free. How about this?
-- >8 -- Subject: [PATCH] cocci: use assignment operator to copy structs
Add a semantic patch for converting memcpy(3) calls targeting addresses of variables (i.e., variables preceded by &) -- which are basically always structs -- to simple assignments, and apply it to the current tree. The resulting code is shorter, simpler and its type safety is checked by the compiler.
Signed-off-by: Rene Scharfe <l.s.r@web.de> --- builtin/log.c | 2 +- contrib/coccinelle/copy.cocci | 31 +++++++++++++++++++++++++++++++ convert.c | 2 +- credential-cache--daemon.c | 2 +- daemon.c | 2 +- line-log.c | 2 +- revision.c | 2 +- 7 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 contrib/coccinelle/copy.cocci
diff --git a/builtin/log.c b/builtin/log.c index 55d20cc2d8..23bb9a9e76 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1030,7 +1030,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, if (!origin) return; - memcpy(&opts, &rev->diffopt, sizeof(opts)); + opts = rev->diffopt; opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT; diff_setup_done(&opts); diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci new file mode 100644 index 0000000000..f0d883932a --- /dev/null +++ b/contrib/coccinelle/copy.cocci @@ -0,0 +1,31 @@ +@@ +type T; +T dst; +T src; +@@ +( +- memcpy(&dst, &src, sizeof(dst)); ++ dst = src; +| +- memcpy(&dst, &src, sizeof(src)); ++ dst = src; +| +- memcpy(&dst, &src, sizeof(T)); ++ dst = src; +) + +@@ +type T; +T dst; +T *src; +@@ +( +- memcpy(&dst, src, sizeof(dst)); ++ dst = *src; +| +- memcpy(&dst, src, sizeof(*src)); ++ dst = *src; +| +- memcpy(&dst, src, sizeof(T)); ++ dst = *src; +) diff --git a/convert.c b/convert.c index 8d652bf27c..4bae12be6b 100644 --- a/convert.c +++ b/convert.c @@ -290,7 +290,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len, if ((checksafe == SAFE_CRLF_WARN || (checksafe == SAFE_CRLF_FAIL)) && len) { struct text_stat new_stats; - memcpy(&new_stats, &stats, sizeof(new_stats)); + new_stats = stats; /* simulate "git add" */ if (convert_crlf_into_lf) { new_stats.lonelf += new_stats.crlf; diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index 46c5937526..798cf33c3a 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -22,7 +22,7 @@ static void cache_credential(struct credential *c, int timeout) e = &entries[entries_nr++]; /* take ownership of pointers */ - memcpy(&e->item, c, sizeof(*c)); + e->item = *c; memset(c, 0, sizeof(*c)); e->expiration = time(NULL) + timeout; } diff --git a/daemon.c b/daemon.c index 473e6b6b63..f891398aad 100644 --- a/daemon.c +++ b/daemon.c @@ -785,7 +785,7 @@ static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_ newborn = xcalloc(1, sizeof(*newborn)); live_children++; - memcpy(&newborn->cld, cld, sizeof(*cld)); + newborn->cld = *cld; memcpy(&newborn->address, addr, addrlen); for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next) if (!addrcmp(&(*cradle)->address, &newborn->address)) diff --git a/line-log.c b/line-log.c index 65f3558b3b..64f141e200 100644 --- a/line-log.c +++ b/line-log.c @@ -1093,7 +1093,7 @@ static int process_all_files(struct line_log_data **range_out, rg = rg->next; assert(rg); rg->pair = diff_filepair_dup(queue->queue[i]); - memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges)); + rg->diff = *pairdiff; } free(pairdiff); } diff --git a/revision.c b/revision.c index b37dbec378..289977c796 100644 --- a/revision.c +++ b/revision.c @@ -2738,7 +2738,7 @@ int prepare_revision_walk(struct rev_info *revs) struct object_array old_pending; struct commit_list **next = &revs->commits; - memcpy(&old_pending, &revs->pending, sizeof(old_pending)); + old_pending = revs->pending; revs->pending.nr = 0; revs->pending.alloc = 0; revs->pending.objects = NULL;
-- 2.12.0