Re: [PATCH 60/68] prefer memcpy to strcpy

From: René Scharfe <l.s.r@web.de>
Date: Sep 27, 2015, 11:19 UTC
Message-ID: <5607D0A9.4000506@web.de>
In-Reply-To: <20150924210818.GE30946@sigill.intra.peff.net>

Am 24.09.2015 um 23:08 schrieb Jeff King:

Show 29 quoted lines

> When we already know the length of a string (e.g., because
> we just malloc'd to fit it), it's nicer to use memcpy than
> strcpy, as it makes it more obvious that we are not going to
> overflow the buffer (because the size we pass matches the
> size in the allocation).
>
> This also eliminates calls to strcpy, which make auditing
> the code base harder.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   compat/nedmalloc/nedmalloc.c | 5 +++--
>   fast-import.c                | 5 +++--
>   revision.c                   | 2 +-
>   3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
> index 609ebba..a0a16eb 100644
> --- a/compat/nedmalloc/nedmalloc.c
> +++ b/compat/nedmalloc/nedmalloc.c
> @@ -957,8 +957,9 @@ char *strdup(const char *s1)
>   {
>   	char *s2 = 0;
>   	if (s1) {
> -		s2 = malloc(strlen(s1) + 1);
> -		strcpy(s2, s1);
> +		size_t len = strlen(s1) + 1;
> +		s2 = malloc(len);
> +		memcpy(s2, s1, len);

This leaves the last byte uninitialized; it was set to NUL by strcpy() before.

Show 16 quoted lines

>   	}
>   	return s2;
>   }
> diff --git a/fast-import.c b/fast-import.c
> index 895c6b4..cf6d8bc 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -644,8 +644,9 @@ static void *pool_calloc(size_t count, size_t size)
>
>   static char *pool_strdup(const char *s)
>   {
> -	char *r = pool_alloc(strlen(s) + 1);
> -	strcpy(r, s);
> +	size_t len = strlen(s) + 1;
> +	char *r = pool_alloc(len);
> +	memcpy(r, s, len);

Same here.

Show 13 quoted lines

>   	return r;
>   }
>
> diff --git a/revision.c b/revision.c
> index af2a18e..2236463 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -38,7 +38,7 @@ char *path_name(const struct name_path *path, const char *name)
>   	}
>   	n = xmalloc(len);
>   	m = n + len - (nlen + 1);
> -	strcpy(m, name);
> +	memcpy(m, name, nlen + 1);

This copies the NUL byte terminating the string, so it's OK. However, I wonder if using a strbuf for building the path in one go instead would simplify this function without too much of a performance impact.

>   	for (p = path; p; p = p->up) {
>   		if (p->elem_len) {
>   			m -= p->elem_len + 1;
>