[PATCH] sha1_name: use strlcpy() to copy strings

[PATCH] sha1_name: use strlcpy() to copy strings

[René Scharfe]

Use strlcpy() instead of calling strncpy() and then setting the last
byte of the target buffer to NUL explicitly.  This shortens and
simplifies the code a bit.

Signed-of-by: Rene Scharfe <l.s.r@web.de>
---
 sha1_name.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index cf2a83b..95f9f8f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1391,9 +1391,7 @@ static int get_sha1_with_context_1(const char *name,
 			namelen = strlen(cp);
 		}
 
-		strncpy(oc->path, cp,
-			sizeof(oc->path));
-		oc->path[sizeof(oc->path)-1] = '\0';
+		strlcpy(oc->path, cp, sizeof(oc->path));
 
 		if (!active_cache)
 			read_cache();
@@ -1443,9 +1441,7 @@ static int get_sha1_with_context_1(const char *name,
 							   name, len);
 			}
 			hashcpy(oc->tree, tree_sha1);
-			strncpy(oc->path, filename,
-				sizeof(oc->path));
-			oc->path[sizeof(oc->path)-1] = '\0';
+			strlcpy(oc->path, filename, sizeof(oc->path));
 
 			free(new_filename);
 			return ret;
-- 
2.3.0

Re: [PATCH] sha1_name: use strlcpy() to copy strings

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Use strlcpy() instead of calling strncpy() and then setting the last
> byte of the target buffer to NUL explicitly.  This shortens and
> simplifies the code a bit.

Thanks.  It makes me wonder if the longer term direction should be
not to use a bound buffer for oc->path, though.

>
> Signed-of-by: Rene Scharfe <l.s.r@web.de>
> ---
>  sha1_name.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index cf2a83b..95f9f8f 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1391,9 +1391,7 @@ static int get_sha1_with_context_1(const char *name,
>  			namelen = strlen(cp);
>  		}
>  
> -		strncpy(oc->path, cp,
> -			sizeof(oc->path));
> -		oc->path[sizeof(oc->path)-1] = '\0';
> +		strlcpy(oc->path, cp, sizeof(oc->path));
>  
>  		if (!active_cache)
>  			read_cache();
> @@ -1443,9 +1441,7 @@ static int get_sha1_with_context_1(const char *name,
>  							   name, len);
>  			}
>  			hashcpy(oc->tree, tree_sha1);
> -			strncpy(oc->path, filename,
> -				sizeof(oc->path));
> -			oc->path[sizeof(oc->path)-1] = '\0';
> +			strlcpy(oc->path, filename, sizeof(oc->path));
>  
>  			free(new_filename);
>  			return ret;

Re: [PATCH] sha1_name: use strlcpy() to copy strings

[René Scharfe]

Am 22.02.2015 um 21:00 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Use strlcpy() instead of calling strncpy() and then setting the last
>> byte of the target buffer to NUL explicitly.  This shortens and
>> simplifies the code a bit.
>
> Thanks.  It makes me wonder if the longer term direction should be
> not to use a bound buffer for oc->path, though.

That's a good idea in general, but a bit more involved since we'd need 
to introduce a cleanup function that releases the memory allocated by 
the new version of get_sha1_with_context() first and call it from the 
appropriate places.

Would that be a good micro-project for GSoC or is it too simple?

René

Re: [PATCH] sha1_name: use strlcpy() to copy strings

[Jeff King]

On Sun, Feb 22, 2015 at 11:33:16PM +0100, René Scharfe wrote:

> Am 22.02.2015 um 21:00 schrieb Junio C Hamano:
> >René Scharfe <l.s.r@web.de> writes:
> >
> >>Use strlcpy() instead of calling strncpy() and then setting the last
> >>byte of the target buffer to NUL explicitly.  This shortens and
> >>simplifies the code a bit.
> >
> >Thanks.  It makes me wonder if the longer term direction should be
> >not to use a bound buffer for oc->path, though.
> 
> That's a good idea in general, but a bit more involved since we'd need to
> introduce a cleanup function that releases the memory allocated by the new
> version of get_sha1_with_context() first and call it from the appropriate
> places.
> 
> Would that be a good micro-project for GSoC or is it too simple?

Yeah, avoiding resource ownership questions was one of the reasons I
went with the static buffer in the first place. But I would love to see
it go away. Not only does it potentially truncate paths, but I recall
there was some complication with the size of "struct object_context" (I
couldn't find the details in a cursory search, but basically it was not
reasonable to have a big array of them).

Could we perhaps make this more like sha1_object_info_extended, where
the caller "asks" for fields by filling in pointers, and the
object_context itself can be discarded without leaking resources?

Like:

  struct strbuf path = STRBUF_INIT;
  struct object_context oc = OBJECT_CONTEXT_INIT;

  oc.path = &path;
  get_sha1_with_context(sha1, &oc);

  ... use path directly ...
  strbuf_release(&path);

Then callers who do not care about the path do not have to even know the
feature exists (and it opens us up to adding new string-like context
fields in the future if we need to).

-Peff