{"thread":{"id":"38627","subject":"[PATCH] sha1_name: use strlcpy() to copy strings","startedAt":"2015-02-21T19:55:22Z","lastAt":"2015-02-23T18:36:09Z","messageCount":4,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"256477","messageId":"54E8E2AA.1020300@web.de","threadId":"38627","inReplyTo":null,"subject":"[PATCH] sha1_name: use strlcpy() to copy strings","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-02-21T19:55:22Z","receivedAt":"2015-02-21T19:55:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use strlcpy() instead of calling strncpy() and then setting the last\nbyte of the target buffer to NUL explicitly. This shortens and\nsimplifies the code a bit.\n\nSigned-of-by: Rene Scharfe \n---\n sha1_name.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex cf2a83b..95f9f8f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1391,9 +1391,7 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\tnamelen = strlen(cp);\n \t\t}\n \n-\t\tstrncpy(oc->path, cp,\n-\t\t\tsizeof(oc->path));\n-\t\toc->path[sizeof(oc->path)-1] = '\\0';\n+\t\tstrlcpy(oc->path, cp, sizeof(oc->path));\n \n \t\tif (!active_cache)\n \t\t\tread_cache();\n@@ -1443,9 +1441,7 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\t\t\t\t\t name, len);\n \t\t\t}\n \t\t\thashcpy(oc->tree, tree_sha1);\n-\t\t\tstrncpy(oc->path, filename,\n-\t\t\t\tsizeof(oc->path));\n-\t\t\toc->path[sizeof(oc->path)-1] = '\\0';\n+\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n \n \t\t\tfree(new_filename);\n \t\t\treturn ret;\n-- \n2.3.0\n"},{"id":"256487","messageId":"xmqqsidxvhrk.fsf@gitster.dls.corp.google.com","threadId":"38627","inReplyTo":"54E8E2AA.1020300@web.de","subject":"Re: [PATCH] sha1_name: use strlcpy() to copy strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-22T20:00:15Z","receivedAt":"2015-02-22T20:00:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Use strlcpy() instead of calling strncpy() and then setting the last\n> byte of the target buffer to NUL explicitly. This shortens and\n> simplifies the code a bit.\n\nThanks. It makes me wonder if the longer term direction should be\nnot to use a bound buffer for oc->path, though.\n\n>\n> Signed-of-by: Rene Scharfe \n> ---\n> sha1_name.c | 8 ++------\n> 1 file changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index cf2a83b..95f9f8f 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1391,9 +1391,7 @@ static int get_sha1_with_context_1(const char *name,\n> \t\t\tnamelen = strlen(cp);\n> \t\t}\n> \n> -\t\tstrncpy(oc->path, cp,\n> -\t\t\tsizeof(oc->path));\n> -\t\toc->path[sizeof(oc->path)-1] = '\\0';\n> +\t\tstrlcpy(oc->path, cp, sizeof(oc->path));\n> \n> \t\tif (!active_cache)\n> \t\t\tread_cache();\n> @@ -1443,9 +1441,7 @@ static int get_sha1_with_context_1(const char *name,\n> \t\t\t\t\t\t\t name, len);\n> \t\t\t}\n> \t\t\thashcpy(oc->tree, tree_sha1);\n> -\t\t\tstrncpy(oc->path, filename,\n> -\t\t\t\tsizeof(oc->path));\n> -\t\t\toc->path[sizeof(oc->path)-1] = '\\0';\n> +\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n> \n> \t\t\tfree(new_filename);\n> \t\t\treturn ret;\n"},{"id":"256489","messageId":"54EA592C.6040701@web.de","threadId":"38627","inReplyTo":"xmqqsidxvhrk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] sha1_name: use strlcpy() to copy strings","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-02-22T22:33:16Z","receivedAt":"2015-02-22T22:33:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.02.2015 um 21:00 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> Use strlcpy() instead of calling strncpy() and then setting the last\n>> byte of the target buffer to NUL explicitly. This shortens and\n>> simplifies the code a bit.\n>\n> Thanks. It makes me wonder if the longer term direction should be\n> not to use a bound buffer for oc->path, though.\n\nThat's a good idea in general, but a bit more involved since we'd need \nto introduce a cleanup function that releases the memory allocated by \nthe new version of get_sha1_with_context() first and call it from the \nappropriate places.\n\nWould that be a good micro-project for GSoC or is it too simple?\n\nRené\n"},{"id":"256528","messageId":"20150223183609.GC19904@peff.net","threadId":"38627","inReplyTo":"54EA592C.6040701@web.de","subject":"Re: [PATCH] sha1_name: use strlcpy() to copy strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-23T18:36:09Z","receivedAt":"2015-02-23T18:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 22, 2015 at 11:33:16PM +0100, René Scharfe wrote:\n\n> Am 22.02.2015 um 21:00 schrieb Junio C Hamano:\n> >René Scharfe writes:\n> >\n> >>Use strlcpy() instead of calling strncpy() and then setting the last\n> >>byte of the target buffer to NUL explicitly. This shortens and\n> >>simplifies the code a bit.\n> >\n> >Thanks. It makes me wonder if the longer term direction should be\n> >not to use a bound buffer for oc->path, though.\n> \n> That's a good idea in general, but a bit more involved since we'd need to\n> introduce a cleanup function that releases the memory allocated by the new\n> version of get_sha1_with_context() first and call it from the appropriate\n> places.\n> \n> Would that be a good micro-project for GSoC or is it too simple?\n\nYeah, avoiding resource ownership questions was one of the reasons I\nwent with the static buffer in the first place. But I would love to see\nit go away. Not only does it potentially truncate paths, but I recall\nthere was some complication with the size of \"struct object_context\" (I\ncouldn't find the details in a cursory search, but basically it was not\nreasonable to have a big array of them).\n\nCould we perhaps make this more like sha1_object_info_extended, where\nthe caller \"asks\" for fields by filling in pointers, and the\nobject_context itself can be discarded without leaking resources?\n\nLike:\n\n struct strbuf path = STRBUF_INIT;\n struct object_context oc = OBJECT_CONTEXT_INIT;\n\n oc.path = &path;\n get_sha1_with_context(sha1, &oc);\n\n ... use path directly ...\n strbuf_release(&path);\n\nThen callers who do not care about the path do not have to even know the\nfeature exists (and it opens us up to adding new string-like context\nfields in the future if we need to).\n\n-Peff\n"}]}