threads / patch / 38627

patchsha1_name: use strlcpy() to copy strings

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

## tl;dr

4 messages between Feb 21, 2015 and Feb 23, 2015. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

René Scharfe· Feb 21, 2015, 19:55 UTC · lore

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(-)

Show changes to sha1_name.c +2 −6

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

Junio C Hamano· Feb 22, 2015, 20:00 UTC · re: René Scharfe · lore

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

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.

Show 32 quoted lines

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

René Scharfe· Feb 22, 2015, 22:33 UTC · re: Junio C Hamano · lore

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

Am 22.02.2015 um 21:00 schrieb Junio C Hamano:

Show 8 quoted lines

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

Jeff King· Feb 23, 2015, 18:36 UTC · re: René Scharfe · lore

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

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

Show 16 quoted lines

> 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