Skip to contentSkip to conversation

sha1_name: use strlcpy() to copy strings

PATCH In review René Scharfe 3 participants 4 messages Jun 15, 2016

lore
Share & tools
lore

René Scharfe

author
#0 lore raw

PATCH 1/1 sha1_name: use strlcpy() to copy strings

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 <[email protected]>
---
 sha1_name.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
sha1_name.c +2 -6
index cf2a83b..95f9f8f 100644
@@ -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;
Signature · 1 line
2.3.0
#1 lore raw
Quoted 3 lines · 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.

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

Quoted 32 lines
Signed-of-by Rene Scharfe <[email protected]>
---
 sha1_name.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
sha1_name.c +2 -6
index cf2a83b..95f9f8f 100644
@@ -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;
author
#2 lore raw

Am 22.02.2015 um 21:00 schrieb Junio C Hamano:

Quoted 8 lines
Quoted 3 lines · 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.

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é

#3 lore raw
Quoted 10 lines · René Scharfe

Am 22.02.2015 um 21:00 schrieb Junio C Hamano:

Quoted 8 lines
Quoted 3 lines · 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.

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

Quoted 6 lines

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