Re: [PATCH] clone: detect errors in normalize_path_copy

From: Stefan Beller <sbeller@google.com>
Date: Oct 5, 2016, 16:48 UTC
Message-ID: <CAGZ79kb_VVoAO0p-XtvZRa3akm+UEMJQjOUip4yrH6m36BVRrg@mail.gmail.com>
In-Reply-To: <20161005142929.l7fy3jxkme4iwunc@sigill.intra.peff.net>

On Wed, Oct 5, 2016 at 7:29 AM, Jeff King <peff@peff.net> wrote:

Show 15 quoted lines

> When we are copying the alternates from the source
> repository, if we find a relative path that is too deep for
> the source (e.g., "../../../objects" from "/repo.git/objects"),
> then normalize_path_copy will report an error and leave
> trash in the buffer, which we will add to our new alternates
> file. Instead, let's detect the error, print a warning, and
> skip copying that alternate.
>
> There's no need to die. The relative path is probably just
> broken cruft in the source repo. If it turns out to have
> been important for accessing some objects, we rely on other
> parts of the clone to detect that, just as they would with a
> missing object in the source repo itself (though note that
> clones with "-s" are inherently local, which may do fewer
> object-quality checks in the first place).

This explanation and the implementation make sense. Thanks!

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Noticed by coverity;

I saw them, too and wanted to start preparing a patch, but I cannot quite compete with your speed here. ;)

Show 29 quoted lines

> the recent alternates cleanups mean that all of the
> other calls to normalize_path_copy() are now checked, so it realized
> this one was an oddball and probably an error (I actually looked for
> others with `grep` when doing that series, but somehow missed this one;
> hooray for static analysis). The fix is independent of that series.
>
>  builtin/clone.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index fb75f7e..6cf3b54 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -345,8 +345,11 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,
>                         continue;
>                 }
>                 abs_path = mkpathdup("%s/objects/%s", src_repo, line.buf);
> -               normalize_path_copy(abs_path, abs_path);
> -               add_to_alternates_file(abs_path);
> +               if (!normalize_path_copy(abs_path, abs_path))
> +                       add_to_alternates_file(abs_path);
> +               else
> +                       warning("skipping invalid relative alternate: %s/%s",
> +                               src_repo, line.buf);
>                 free(abs_path);
>         }
>         strbuf_release(&line);
> --
> 2.10.1.506.g904834d