Am 05.03.2017 um 12:36 schrieb Jeff King:
> 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:
> 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@xxxxxx>
---
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