{"thread":{"id":"19420","subject":"[PATCH] setup_revisions(): do not access outside argv","startedAt":"2009-05-20T08:08:20Z","lastAt":"2009-06-02T13:57:52Z","messageCount":16,"participants":["Nguyễn Thái Ngọc Duy","Johannes Sixt","Nguyen Thai Ngoc Duy","Junio C Hamano","Miles Bader","Jeff King","Thomas Jarosch","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"114322","messageId":"1242806900-3499-1-git-send-email-pclouds@gmail.com","threadId":"19420","inReplyTo":null,"subject":"[PATCH] setup_revisions(): do not access outside argv","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-05-20T08:08:20Z","receivedAt":"2009-05-20T08:08:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n revision.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 18b7ebb..be1e307 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\tif (strcmp(arg, \"--\"))\n \t\t\tcontinue;\n \t\targv[i] = NULL;\n-\t\targc = i;\n-\t\tif (argv[i + 1])\n+\t\tif (i + 1 < argc && argv[i + 1])\n \t\t\trevs->prune_data = get_pathspec(revs->prefix, argv + i + 1);\n+\t\targc = i;\n \t\tseen_dashdash = 1;\n \t\tbreak;\n \t}\n-- \ntest\n"},{"id":"114324","messageId":"4A13BC3C.5070000@viscovery.net","threadId":"19420","inReplyTo":"1242806900-3499-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-05-20T08:15:56Z","receivedAt":"2009-05-20T08:15:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nguyễn Thái Ngọc Duy schrieb:\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> revision.c | 4 ++--\n> 1 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index 18b7ebb..be1e307 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n> \t\tif (strcmp(arg, \"--\"))\n> \t\t\tcontinue;\n> \t\targv[i] = NULL;\n> -\t\targc = i;\n> -\t\tif (argv[i + 1])\n> +\t\tif (i + 1 < argc && argv[i + 1])\n> \t\t\trevs->prune_data = get_pathspec(revs->prefix, argv + i + 1);\n> +\t\targc = i;\n> \t\tseen_dashdash = 1;\n> \t\tbreak;\n> \t}\n\nWhy is this necessary? I'd expect that argv arrays have NULL at the end.\nIf this is a bug, why did noboy notice this earlier? IOW, your commit\nmessage is really lacking justification.\n\n-- Hannes\n"},{"id":"114325","messageId":"fcaeb9bf0905200123r3649a7e5vc40ece402379e701@mail.gmail.com","threadId":"19420","inReplyTo":"4A13BC3C.5070000@viscovery.net","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-05-20T08:23:27Z","receivedAt":"2009-05-20T08:23:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2009/5/20 Johannes Sixt :\n> Nguyễn Thái Ngọc Duy schrieb:\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>>  revision.c |    4 ++--\n>>  1 files changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/revision.c b/revision.c\n>> index 18b7ebb..be1e307 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n>>               if (strcmp(arg, \"--\"))\n>>                       continue;\n>>               argv[i] = NULL;\n>> -             argc = i;\n>> -             if (argv[i + 1])\n>> +             if (i + 1 < argc && argv[i + 1])\n>>                       revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);\n>> +             argc = i;\n>>               seen_dashdash = 1;\n>>               break;\n>>       }\n>\n> Why is this necessary? I'd expect that argv arrays have NULL at the end.\n\nI have no idea. I hit this \"bug\" in my builtin-rebase.c and had that\nquestion too. But I grepped through and saw that\nat least verify_bundle() does not terminate argv with NULL. So I\nassume that setup_revisions() does not expect NULL at the end.\n-- \nDuy\n"},{"id":"114382","messageId":"7v7i0btdwu.fsf@alter.siamese.dyndns.org","threadId":"19420","inReplyTo":"fcaeb9bf0905200123r3649a7e5vc40ece402379e701@mail.gmail.com","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-21T01:58:41Z","receivedAt":"2009-05-21T01:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> 2009/5/20 Johannes Sixt :\n>> Nguyễn Thái Ngọc Duy schrieb:\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>> ---\n>>> revision.c | 4 ++--\n>>> 1 files changed, 2 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/revision.c b/revision.c\n>>> index 18b7ebb..be1e307 100644\n>>> --- a/revision.c\n>>> +++ b/revision.c\n>>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n>>> if (strcmp(arg, \"--\"))\n>>> continue;\n>>> argv[i] = NULL;\n>>> - argc = i;\n>>> - if (argv[i + 1])\n>>> + if (i + 1 < argc && argv[i + 1])\n>>> revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);\n>>> + argc = i;\n>>> seen_dashdash = 1;\n>>> break;\n>>> }\n>>\n>> Why is this necessary? I'd expect that argv arrays have NULL at the end.\n>\n> I have no idea. I hit this \"bug\" in my builtin-rebase.c and had that\n> question too. But I grepped through and saw that\n> at least verify_bundle() does not terminate argv with NULL. So I\n> assume that setup_revisions() does not expect NULL at the end.\n\nIf a function takes (int ac, char **av), then people should be able to\ndepend on the usual convention of\n\n (1) for any i < ac, av[i] is not NULL; and\n (2) av[ac] is NULL.\n\nWith your patch, a broken caller's wish is simply discarded and nobody\nwill notice. Without your patch, at least you will know that the caller\npassed an inconsistent pair of ac and av to this function by seeing a\ncoalmine canary segfault.\n\nI would not mind a patch that adds an assertion that protects this\nfunction from broken callers, so that we can find them, but your patch\nmakes me feel very uneasy.\n"},{"id":"114383","messageId":"buoljor6uzi.fsf@dhlpc061.dev.necel.com","threadId":"19420","inReplyTo":"7v7i0btdwu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2009-05-21T02:38:25Z","receivedAt":"2009-05-21T02:38:25Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Junio C Hamano writes:\n> If a function takes (int ac, char **av), then people should be able to\n> depend on the usual convention of\n>\n> (1) for any i < ac, av[i] is not NULL; and\n> (2) av[ac] is NULL.\n\nHmm, isn't potentially useful to be able to pass a sub-range (of a\nlonger argv vector) to an ac/av function? In such a case, av[ac] may\nnot be NULL.\n\n-Miles\n\n-- \n97% of everything is grunge\n"},{"id":"114384","messageId":"fcaeb9bf0905201941u2480e102jd1925593e288b608@mail.gmail.com","threadId":"19420","inReplyTo":"7v7i0btdwu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2009-05-21T02:41:54Z","receivedAt":"2009-05-21T02:41:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 21, 2009 at 11:58 AM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>> 2009/5/20 Johannes Sixt :\n>>> Nguyễn Thái Ngọc Duy schrieb:\n>>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>>> ---\n>>>>  revision.c |    4 ++--\n>>>>  1 files changed, 2 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/revision.c b/revision.c\n>>>> index 18b7ebb..be1e307 100644\n>>>> --- a/revision.c\n>>>> +++ b/revision.c\n>>>> @@ -1241,9 +1241,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n>>>>               if (strcmp(arg, \"--\"))\n>>>>                       continue;\n>>>>               argv[i] = NULL;\n>>>> -             argc = i;\n>>>> -             if (argv[i + 1])\n>>>> +             if (i + 1 < argc && argv[i + 1])\n>>>>                       revs->prune_data = get_pathspec(revs->prefix, argv + i + 1);\n>>>> +             argc = i;\n>>>>               seen_dashdash = 1;\n>>>>               break;\n>>>>       }\n>>>\n>>> Why is this necessary? I'd expect that argv arrays have NULL at the end.\n>>\n>> I have no idea. I hit this \"bug\" in my builtin-rebase.c and had that\n>> question too. But I grepped through and saw that\n>> at least verify_bundle() does not terminate argv with NULL. So I\n>> assume that setup_revisions() does not expect NULL at the end.\n>\n> If a function takes (int ac, char **av), then people should be able to\n> depend on the usual convention of\n>\n>  (1) for any i < ac, av[i] is not NULL; and\n>  (2) av[ac] is NULL.\n>\n> With your patch, a broken caller's wish is simply discarded and nobody\n> will notice.  Without your patch, at least you will know that the caller\n> passed an inconsistent pair of ac and av to this function by seeing a\n> coalmine canary segfault.\n\nOK. I will send another patch for verify_bundle() then :-) just to\nmake sure no one goes down the same way.\n-- \nDuy\n"},{"id":"114390","messageId":"20090521041812.GE8091@sigill.intra.peff.net","threadId":"19420","inReplyTo":"7v7i0btdwu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-21T04:18:12Z","receivedAt":"2009-05-21T04:18:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 20, 2009 at 06:58:41PM -0700, Junio C Hamano wrote:\n\n> If a function takes (int ac, char **av), then people should be able to\n> depend on the usual convention of\n> \n> (1) for any i < ac, av[i] is not NULL; and\n> (2) av[ac] is NULL.\n> \n> With your patch, a broken caller's wish is simply discarded and nobody\n> will notice. Without your patch, at least you will know that the caller\n> passed an inconsistent pair of ac and av to this function by seeing a\n> coalmine canary segfault.\n> \n> I would not mind a patch that adds an assertion that protects this\n> function from broken callers, so that we can find them, but your patch\n> makes me feel very uneasy.\n\nI agree.\n\nHaving just fixed a segfault in the GIT_TRACE code caused by a\nnon-terminated argv generated by the alias code, I think I would prefer\nthat we just consistently do the NULL-termination. You are otherwise\ncreating a maintenance pitfall when somebody later passes the value to\nunsuspecting code.\n\n-Peff\n"},{"id":"114436","messageId":"4A159720.3020103@intra2net.com","threadId":"19420","inReplyTo":"20090521041812.GE8091@sigill.intra.peff.net","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-05-21T18:02:08Z","receivedAt":"2009-05-21T18:02:08Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"Jeff King wrote:\n> Having just fixed a segfault in the GIT_TRACE code caused by a\n> non-terminated argv generated by the alias code, I think I would prefer\n> that we just consistently do the NULL-termination. You are otherwise\n> creating a maintenance pitfall when somebody later passes the value to\n> unsuspecting code.\n\nSpeaking of that, there is also one piece of code in diff.c that doesn't do\nNULL-termination after a readlink() call (which never NULL-terminates).\nThe current use is 100% fine, though the same maintenance\nargument might apply here, too. Wondering why the buffer\nis allocated as PATH_MAX +1. Hmm.\n\nThomas\n"},{"id":"114454","messageId":"20090522075620.GC1409@coredump.intra.peff.net","threadId":"19420","inReplyTo":"4A159720.3020103@intra2net.com","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-22T07:56:20Z","receivedAt":"2009-05-22T07:56:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 21, 2009 at 08:02:08PM +0200, Thomas Jarosch wrote:\n\n> Speaking of that, there is also one piece of code in diff.c that doesn't do\n> NULL-termination after a readlink() call (which never NULL-terminates).\n> The current use is 100% fine, though the same maintenance\n> argument might apply here, too. Wondering why the buffer\n> is allocated as PATH_MAX +1. Hmm.\n\nYeah, it is fine because it just passes the result to prep_temp_blob,\nwhich respects the length. I don't know if it is worth making it more\nsafe (arguably it should just be using strbuf_readlink anyway, but that\ndoes introduce an extra malloc).\n\nI grepped and every other call to readlink is already doing this (and\nmost just use strbuf_readlink anyway).\n\n-- >8 --\nSubject: NUL-terminate readlink results\n\nreadlink does not terminate its result, but instead returns the length\nof the path. This is not an actual bugfix, as the value is currently\nonly used with its length. However, terminating the string helps make it\nsafer for future uses.\n\nSigned-off-by: Jeff King \n---\nThis does feel a bit like code churn, but I'm not sure it is any\ndifferent than the NULL-terminate all argv proposal.\n\ndiff --git a/diff.c b/diff.c\nindex f06876b..b398360 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2021,6 +2021,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t\t\tdie(\"readlink(%s)\", name);\n \t\t\tif (ret == sizeof(buf))\n \t\t\t\tdie(\"symlink too long: %s\", name);\n+\t\t\tbuf[ret] = '\\0';\n \t\t\tprep_temp_blob(name, temp, buf, ret,\n \t\t\t\t (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n"},{"id":"114455","messageId":"20090522080258.GD1409@coredump.intra.peff.net","threadId":"19420","inReplyTo":"20090522075620.GC1409@coredump.intra.peff.net","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-22T08:02:58Z","receivedAt":"2009-05-22T08:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2009 at 03:56:20AM -0400, Jeff King wrote:\n\n> Yeah, it is fine because it just passes the result to prep_temp_blob,\n> which respects the length. I don't know if it is worth making it more\n> safe (arguably it should just be using strbuf_readlink anyway, but that\n> does introduce an extra malloc).\n\nAnd here is the strbuf_readlink version, which actually does make the\nsource shorter and easier to read.\n\n-- >8 --\nSubject: [PATCH] convert bare readlink to strbuf_readlink\n\nThis particular readlink call never NUL-terminated its\nresult, making it a potential source of bugs (though there\nis no bug now, as it currently always respects the length\nfield). Let's just switch it to strbuf_readlink which is\nshorter and less error-prone.\n\nSigned-off-by: Jeff King \n---\n diff.c | 10 +++-------\n 1 files changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f06876b..ffbe5c4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2014,14 +2014,10 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t\tdie(\"stat(%s): %s\", name, strerror(errno));\n \t\t}\n \t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tint ret;\n-\t\t\tchar buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */\n-\t\t\tret = readlink(name, buf, sizeof(buf));\n-\t\t\tif (ret < 0)\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tif (strbuf_readlink(&sb, name, st.st_size) < 0)\n \t\t\t\tdie(\"readlink(%s)\", name);\n-\t\t\tif (ret == sizeof(buf))\n-\t\t\t\tdie(\"symlink too long: %s\", name);\n-\t\t\tprep_temp_blob(name, temp, buf, ret,\n+\t\t\tprep_temp_blob(name, temp, sb.buf, sb.len,\n \t\t\t\t (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n \t\t\t\t (one->sha1_valid ?\n-- \n1.6.3.1.179.gec578.dirty\n"},{"id":"114468","messageId":"200905221625.07628.thomas.jarosch@intra2net.com","threadId":"19420","inReplyTo":"20090522080258.GD1409@coredump.intra.peff.net","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Thomas Jarosch","fromEmail":"thomas.jarosch@intra2net.com","sentAt":"2009-05-22T14:23:44Z","receivedAt":"2009-05-22T14:23:44Z","isPatch":true,"sender":{"key":"thomas.jarosch@intra2net.com","avatar":"https://avatars.githubusercontent.com/u/1146758?v=4"},"body":"On Friday, 22. May 2009 10:02:58 Jeff King wrote:\n> On Fri, May 22, 2009 at 03:56:20AM -0400, Jeff King wrote:\n> > Yeah, it is fine because it just passes the result to prep_temp_blob,\n> > which respects the length. I don't know if it is worth making it more\n> > safe (arguably it should just be using strbuf_readlink anyway, but that\n> > does introduce an extra malloc).\n>\n> And here is the strbuf_readlink version, which actually does make the\n> source shorter and easier to read.\n\nGood work! Patch looks fine to me. Guess you can't even benchmark\nthe \"extra\" malloc ;-)\n\nHave a nice weekend,\nThomas\n"},{"id":"114470","messageId":"Ys7Cih8N_SClhy9WmlLefLAxz2_XjZb3KAO1jrRMNrMcLq4T98MuIA@cipher.nrlssc.navy.mil","threadId":"19420","inReplyTo":"20090522080258.GD1409@coredump.intra.peff.net","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-05-22T15:33:22Z","receivedAt":"2009-05-22T15:33:22Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Jeff King wrote:\n> On Fri, May 22, 2009 at 03:56:20AM -0400, Jeff King wrote:\n> \n>> Yeah, it is fine because it just passes the result to prep_temp_blob,\n>> which respects the length. I don't know if it is worth making it more\n>> safe (arguably it should just be using strbuf_readlink anyway, but that\n>> does introduce an extra malloc).\n> \n> And here is the strbuf_readlink version, which actually does make the\n> source shorter and easier to read.\n> \n> -- >8 --\n> Subject: [PATCH] convert bare readlink to strbuf_readlink\n> \n> This particular readlink call never NUL-terminated its\n> result, making it a potential source of bugs (though there\n> is no bug now, as it currently always respects the length\n> field). Let's just switch it to strbuf_readlink which is\n> shorter and less error-prone.\n> \n> Signed-off-by: Jeff King \n> ---\n> diff.c | 10 +++-------\n> 1 files changed, 3 insertions(+), 7 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index f06876b..ffbe5c4 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2014,14 +2014,10 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n> \t\t\tdie(\"stat(%s): %s\", name, strerror(errno));\n> \t\t}\n> \t\tif (S_ISLNK(st.st_mode)) {\n> -\t\t\tint ret;\n> -\t\t\tchar buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */\n> -\t\t\tret = readlink(name, buf, sizeof(buf));\n> -\t\t\tif (ret < 0)\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tif (strbuf_readlink(&sb, name, st.st_size) < 0)\n> \t\t\t\tdie(\"readlink(%s)\", name);\n> -\t\t\tif (ret == sizeof(buf))\n> -\t\t\t\tdie(\"symlink too long: %s\", name);\n> -\t\t\tprep_temp_blob(name, temp, buf, ret,\n> +\t\t\tprep_temp_blob(name, temp, sb.buf, sb.len,\n> \t\t\t\t (one->sha1_valid ?\n> \t\t\t\t\tone->sha1 : null_sha1),\n> \t\t\t\t (one->sha1_valid ?\n\nDon't you need to strbuf_release() ?\n\n-brandon\n"},{"id":"114471","messageId":"20090522153426.GA10390@coredump.intra.peff.net","threadId":"19420","inReplyTo":"Ys7Cih8N_SClhy9WmlLefLAxz2_XjZb3KAO1jrRMNrMcLq4T98MuIA@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-22T15:34:26Z","receivedAt":"2009-05-22T15:34:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 22, 2009 at 10:33:22AM -0500, Brandon Casey wrote:\n\n> > \t\tif (S_ISLNK(st.st_mode)) {\n> > -\t\t\tint ret;\n> > -\t\t\tchar buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */\n> > -\t\t\tret = readlink(name, buf, sizeof(buf));\n> > -\t\t\tif (ret < 0)\n> > +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> > +\t\t\tif (strbuf_readlink(&sb, name, st.st_size) < 0)\n> > \t\t\t\tdie(\"readlink(%s)\", name);\n> > -\t\t\tif (ret == sizeof(buf))\n> > -\t\t\t\tdie(\"symlink too long: %s\", name);\n> > -\t\t\tprep_temp_blob(name, temp, buf, ret,\n> > +\t\t\tprep_temp_blob(name, temp, sb.buf, sb.len,\n> > \t\t\t\t (one->sha1_valid ?\n> > \t\t\t\t\tone->sha1 : null_sha1),\n> > \t\t\t\t (one->sha1_valid ?\n> \n> Don't you need to strbuf_release() ?\n\nUrgh, yes, of course. Thanks for noticing.\n\n-Peff\n"},{"id":"114655","messageId":"20090525104609.GA26895@coredump.intra.peff.net","threadId":"19420","inReplyTo":"20090522153426.GA10390@coredump.intra.peff.net","subject":"[PATCH] convert bare readlink to strbuf_readlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-05-25T10:46:09Z","receivedAt":"2009-05-25T10:46:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This particular readlink call never NUL-terminated its\nresult, making it a potential source of bugs (though there\nis no bug now, as it currently always respects the length\nfield). Let's just switch it to strbuf_readlink which is\nshorter and less error-prone.\n\nSigned-off-by: Jeff King \n---\nThis is a re-post with the missing strbuf_release added.\n\nI am not overly married to this patch, so if you think it is code churn,\nplease just say so and drop it. I am just clearing out my \"clean up and\nsubmit\" queue. :)\n\n diff.c | 11 ++++-------\n 1 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f06876b..dcfbcb0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2014,18 +2014,15 @@ static struct diff_tempfile *prepare_temp_file(const char *name,\n \t\t\tdie(\"stat(%s): %s\", name, strerror(errno));\n \t\t}\n \t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tint ret;\n-\t\t\tchar buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */\n-\t\t\tret = readlink(name, buf, sizeof(buf));\n-\t\t\tif (ret < 0)\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tif (strbuf_readlink(&sb, name, st.st_size) < 0)\n \t\t\t\tdie(\"readlink(%s)\", name);\n-\t\t\tif (ret == sizeof(buf))\n-\t\t\t\tdie(\"symlink too long: %s\", name);\n-\t\t\tprep_temp_blob(name, temp, buf, ret,\n+\t\t\tprep_temp_blob(name, temp, sb.buf, sb.len,\n \t\t\t\t (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n \t\t\t\t (one->sha1_valid ?\n \t\t\t\t\tone->mode : S_IFLNK));\n+\t\t\tstrbuf_release(&sb);\n \t\t}\n \t\telse {\n \t\t\t/* we can borrow from the file in the work tree */\n-- \n1.6.3.1.250.g01b8b.dirty\n"},{"id":"114713","messageId":"7v1vqcvn2e.fsf@alter.siamese.dyndns.org","threadId":"19420","inReplyTo":"20090525104609.GA26895@coredump.intra.peff.net","subject":"Re: [PATCH] convert bare readlink to strbuf_readlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-25T22:23:53Z","receivedAt":"2009-05-25T22:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is a re-post with the missing strbuf_release added.\n\nThanks.\n"},{"id":"115245","messageId":"20090602135751.GA5594@coredump.intra.peff.net","threadId":"19420","inReplyTo":"20090602195605.6117@nanako3.lavabit.com","subject":"Re: [PATCH] setup_revisions(): do not access outside argv","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-06-02T13:57:52Z","receivedAt":"2009-06-02T13:57:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 02, 2009 at 07:56:05PM +0900, Nanako Shiraishi wrote:\n\n> > And here is the strbuf_readlink version, which actually does make the\n> > source shorter and easier to read.\n> \n> Junio, may I ask what happened to this patch?\n\nI think it's now 3cd7388.\n\n-Peff\n"}]}