{"thread":{"id":"15896","subject":"[PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c","startedAt":"2008-10-14T16:23:49Z","lastAt":"2008-10-16T06:21:12Z","messageCount":5,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93016","messageId":"81b0412b0810140923x5cf58bb9x5acd1517a19e9847@mail.gmail.com","threadId":"15896","inReplyTo":null,"subject":"[PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-14T16:23:49Z","receivedAt":"2008-10-14T16:23:49Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Otherwise the function sometimes fail to resolve obviously correct refnames,\nbecause the string data pointed to by \"ref\" argument were reused.\n\nSigned-off-by: Alex Riesen \n---\n sha1_name.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\nFrankly, I'm having hard time trying to justify _any_ use of mkpath.\nIt is a bug waiting to happen every time is any code between the\ncall site and assignment but strdup.\n\n\nFrom 052e137d4ad8687d78bb2570390ed4f6b19f7cba Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 14 Oct 2008 18:14:20 +0200\nSubject: [PATCH] Fix mkpath abuse in sha1_name.c\n\nOtherwise the function sometimes fail to resolve obviously correct refnames,\nbecause the string data pointed to by \"ref\" argument were reused.\n\nSigned-off-by: Alex Riesen \n---\n sha1_name.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 41b6809..b5b53bf 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -242,6 +242,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n {\n \tconst char **p, *r;\n \tint refs_found = 0;\n+\tchar fullref[PATH_MAX];\n \n \t*ref = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n@@ -249,7 +250,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \t\tunsigned char *this_result;\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n-\t\tr = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);\n+\t\tsnprintf(fullref, sizeof(fullref), *p, len, str);\n+\t\tr = resolve_ref(fullref, this_result, 1, NULL);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -272,7 +274,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n-\t\tstrcpy(path, mkpath(*p, len, str));\n+\t\tsnprintf(path, sizeof(path), *p, len, str);\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-- \n1.6.0.2.494.gb25da\n\n"},{"id":"93044","messageId":"7viqru6a1r.fsf@gitster.siamese.dyndns.org","threadId":"15896","inReplyTo":"81b0412b0810140923x5cf58bb9x5acd1517a19e9847@mail.gmail.com","subject":"Re: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-14T23:17:52Z","receivedAt":"2008-10-14T23:17:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> From: Alex Riesen \n> Date: Tue, 14 Oct 2008 18:14:20 +0200\n> Subject: [PATCH] Fix mkpath abuse in sha1_name.c\n>\n> Otherwise the function sometimes fail to resolve obviously correct refnames,\n> because the string data pointed to by \"ref\" argument were reused.\n>\n> Signed-off-by: Alex Riesen \n\nThe get_pathname() interface has been handy but it indeed is a bug waiting\nto happen, and we have fixed occasional breakages by fixing the earlier\ncallers to stash the returned value away to keep it from getting\noverwritten. I.e. when we have:\n\n\tstr1 = function_that_uses_get_pathname();\n ...\n str2 = another_function_that_uses_get_pathname();\n some_processing(str1, str2);\n\nand we have too many other calls to functions that use get_pathname() in\nthe ... section, str1 will become invalid when we try to feed it to\nsome_processing(). So far our strategy to fix this kind of breakages has\nbeen to rewrite the above to:\n\n\tstr1 = xstrdup(function_that_uses_get_pathname());\n ...\n str2 = another_function_that_uses_get_pathname();\n some_processing(str1, str2);\n\tfree(str1);\n\nBut your patch instead rewrites the computation of str2 by bypassing the\ncall to \"another_function_that_uses_get_pathname()\" and duplicating its\nlogic, which I do not think is a viable approach in the longer term.\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 41b6809..b5b53bf 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -242,6 +242,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n> {\n> \tconst char **p, *r;\n> \tint refs_found = 0;\n> +\tchar fullref[PATH_MAX];\n> \n> \t*ref = NULL;\n> \tfor (p = ref_rev_parse_rules; *p; p++) {\n> @@ -249,7 +250,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n> \t\tunsigned char *this_result;\n> \n> \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n> -\t\tr = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);\n> +\t\tsnprintf(fullref, sizeof(fullref), *p, len, str);\n> +\t\tr = resolve_ref(fullref, this_result, 1, NULL);\n> \t\tif (r) {\n> \t\t\tif (!refs_found++)\n> \t\t\t\t*ref = xstrdup(r);\n\nI suspect that I am grossly misleading the code, but I wonder why this xstrdup()\nis not protecting us from the reusing of \"the string data pointed to by\n\"ref\" argument\". Are you fixing the overwriting of the string pointed to\nby \"str\" argument instead?\n\nWhat specific call chain has this breakage you found?\n"},{"id":"93054","messageId":"20081015062039.GA3775@blimp.localdomain","threadId":"15896","inReplyTo":"7viqru6a1r.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-15T06:20:40Z","receivedAt":"2008-10-15T06:20:40Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Wed, Oct 15, 2008 01:17:52 +0200:\n> \"Alex Riesen\" writes:\n> >\n> > Otherwise the function sometimes fail to resolve obviously correct refnames,\n> > because the string data pointed to by \"ref\" argument were reused.\n> >\n> But your patch instead rewrites the computation of str2 by bypassing the\n> call to \"another_function_that_uses_get_pathname()\" and duplicating its\n> logic, which I do not think is a viable approach in the longer term.\n\nThere is not enough logic to bypass there. It's just a dumb sprintf!\nI actually saved a useless call (two, if automatic inlining\noptimization doesn't work).\n\n> > diff --git a/sha1_name.c b/sha1_name.c\n> > index 41b6809..b5b53bf 100644\n> > --- a/sha1_name.c\n> > +++ b/sha1_name.c\n> > @@ -242,6 +242,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n> > {\n> > \tconst char **p, *r;\n> > \tint refs_found = 0;\n> > +\tchar fullref[PATH_MAX];\n> > \n> > \t*ref = NULL;\n> > \tfor (p = ref_rev_parse_rules; *p; p++) {\n> > @@ -249,7 +250,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n> > \t\tunsigned char *this_result;\n> > \n> > \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n> > -\t\tr = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);\n> > +\t\tsnprintf(fullref, sizeof(fullref), *p, len, str);\n> > +\t\tr = resolve_ref(fullref, this_result, 1, NULL);\n> > \t\tif (r) {\n> > \t\t\tif (!refs_found++)\n> > \t\t\t\t*ref = xstrdup(r);\n> \n> I suspect that I am grossly misleading the code, but I wonder why this xstrdup()\n> is not protecting us from the reusing of \"the string data pointed to by\n> \"ref\" argument\". Are you fixing the overwriting of the string pointed to\n> by \"str\" argument instead?\n\nYes, I mistyped. xstrdup's called after the breakage happened.\n\n> What specific call chain has this breakage you found?\n\nThat's up to this function from builtin_rev_parse and resolve_ref\nwe're about to call.\n"},{"id":"93150","messageId":"7vabd5r1ss.fsf@gitster.siamese.dyndns.org","threadId":"15896","inReplyTo":"20081015062039.GA3775@blimp.localdomain","subject":"Re: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-15T21:22:43Z","receivedAt":"2008-10-15T21:22:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Junio C Hamano, Wed, Oct 15, 2008 01:17:52 +0200:\n>> \"Alex Riesen\" writes:\n>> >\n>> > Otherwise the function sometimes fail to resolve obviously correct refnames,\n>> > because the string data pointed to by \"ref\" argument were reused.\n>> >\n>> But your patch instead rewrites the computation of str2 by bypassing the\n>> call to \"another_function_that_uses_get_pathname()\" and duplicating its\n>> logic, which I do not think is a viable approach in the longer term.\n>\n> There is not enough logic to bypass there. It's just a dumb sprintf!\n\nBut didn't you lose call to cleanup_path()?\n"},{"id":"93166","messageId":"81b0412b0810152321u508f1e2eqa7790baf476baf34@mail.gmail.com","threadId":"15896","inReplyTo":"7vabd5r1ss.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-10-16T06:21:12Z","receivedAt":"2008-10-16T06:21:12Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/10/15 Junio C Hamano :\n> Alex Riesen writes:\n>\n>> Junio C Hamano, Wed, Oct 15, 2008 01:17:52 +0200:\n>>> \"Alex Riesen\" writes:\n>>> >\n>>> > Otherwise the function sometimes fail to resolve obviously correct refnames,\n>>> > because the string data pointed to by \"ref\" argument were reused.\n>>> >\n>>> But your patch instead rewrites the computation of str2 by bypassing the\n>>> call to \"another_function_that_uses_get_pathname()\" and duplicating its\n>>> logic, which I do not think is a viable approach in the longer term.\n>>\n>> There is not enough logic to bypass there. It's just a dumb sprintf!\n>\n> But didn't you lose call to cleanup_path()?\n>\n\nIn this case?\n"}]}