Re: [PATCH v1] Travis: also test on 32-bit Linux

["René Scharfe" <l.s.r@web.de>]

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@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