{"thread":{"id":"44223","subject":"[PATCH] clone: detect errors in normalize_path_copy","startedAt":"2016-10-05T14:29:36Z","lastAt":"2016-10-05T16:48:38Z","messageCount":2,"participants":["Jeff King","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"303367","messageId":"20161005142929.l7fy3jxkme4iwunc@sigill.intra.peff.net","threadId":"44223","inReplyTo":null,"subject":"[PATCH] clone: detect errors in normalize_path_copy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-05T14:29:29Z","receivedAt":"2016-10-05T14:29:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are copying the alternates from the source\nrepository, if we find a relative path that is too deep for\nthe source (e.g., \"../../../objects\" from \"/repo.git/objects\"),\nthen normalize_path_copy will report an error and leave\ntrash in the buffer, which we will add to our new alternates\nfile. Instead, let's detect the error, print a warning, and\nskip copying that alternate.\n\nThere's no need to die. The relative path is probably just\nbroken cruft in the source repo. If it turns out to have\nbeen important for accessing some objects, we rely on other\nparts of the clone to detect that, just as they would with a\nmissing object in the source repo itself (though note that\nclones with \"-s\" are inherently local, which may do fewer\nobject-quality checks in the first place).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNoticed by coverity; the recent alternates cleanups mean that all of the\nother calls to normalize_path_copy() are now checked, so it realized\nthis one was an oddball and probably an error (I actually looked for\nothers with `grep` when doing that series, but somehow missed this one;\nhooray for static analysis). The fix is independent of that series.\n\n builtin/clone.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex fb75f7e..6cf3b54 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -345,8 +345,11 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n \t\t\tcontinue;\n \t\t}\n \t\tabs_path = mkpathdup(\"%s/objects/%s\", src_repo, line.buf);\n-\t\tnormalize_path_copy(abs_path, abs_path);\n-\t\tadd_to_alternates_file(abs_path);\n+\t\tif (!normalize_path_copy(abs_path, abs_path))\n+\t\t\tadd_to_alternates_file(abs_path);\n+\t\telse\n+\t\t\twarning(\"skipping invalid relative alternate: %s/%s\",\n+\t\t\t\tsrc_repo, line.buf);\n \t\tfree(abs_path);\n \t}\n \tstrbuf_release(&line);\n-- \n2.10.1.506.g904834d\n"},{"id":"303389","messageId":"CAGZ79kb_VVoAO0p-XtvZRa3akm+UEMJQjOUip4yrH6m36BVRrg@mail.gmail.com","threadId":"44223","inReplyTo":"20161005142929.l7fy3jxkme4iwunc@sigill.intra.peff.net","subject":"Re: [PATCH] clone: detect errors in normalize_path_copy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-05T16:48:32Z","receivedAt":"2016-10-05T16:48:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 5, 2016 at 7:29 AM, Jeff King <peff@peff.net> wrote:\n> When we are copying the alternates from the source\n> repository, if we find a relative path that is too deep for\n> the source (e.g., \"../../../objects\" from \"/repo.git/objects\"),\n> then normalize_path_copy will report an error and leave\n> trash in the buffer, which we will add to our new alternates\n> file. Instead, let's detect the error, print a warning, and\n> skip copying that alternate.\n>\n> There's no need to die. The relative path is probably just\n> broken cruft in the source repo. If it turns out to have\n> been important for accessing some objects, we rely on other\n> parts of the clone to detect that, just as they would with a\n> missing object in the source repo itself (though note that\n> clones with \"-s\" are inherently local, which may do fewer\n> object-quality checks in the first place).\n\nThis explanation and the implementation make sense.\nThanks!\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Noticed by coverity;\n\nI saw them, too and wanted to start preparing a patch,\nbut I cannot quite compete with your speed here. ;)\n\n> the recent alternates cleanups mean that all of the\n> other calls to normalize_path_copy() are now checked, so it realized\n> this one was an oddball and probably an error (I actually looked for\n> others with `grep` when doing that series, but somehow missed this one;\n> hooray for static analysis). The fix is independent of that series.\n>\n>  builtin/clone.c | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index fb75f7e..6cf3b54 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -345,8 +345,11 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n>                         continue;\n>                 }\n>                 abs_path = mkpathdup(\"%s/objects/%s\", src_repo, line.buf);\n> -               normalize_path_copy(abs_path, abs_path);\n> -               add_to_alternates_file(abs_path);\n> +               if (!normalize_path_copy(abs_path, abs_path))\n> +                       add_to_alternates_file(abs_path);\n> +               else\n> +                       warning(\"skipping invalid relative alternate: %s/%s\",\n> +                               src_repo, line.buf);\n>                 free(abs_path);\n>         }\n>         strbuf_release(&line);\n> --\n> 2.10.1.506.g904834d\n"}]}