From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 25 Jan 2017 18:26:36 GMT
Subject: Re: [PATCH 01/12] for_each_alternate_ref: handle failure from real_pathdup()
Message-ID: <xmqqziifatcz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170124003827.l2rimgitsyxsvtly@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> In older versions of git, if real_path() failed to resolve
> the alternate object store path, we would die() with an
> error. However, since 4ac9006f8 (real_path: have callers use
> real_pathdup and strbuf_realpath, 2016-12-12) we use the
> real_pathdup() function, which may return NULL. Since we
> don't check the return value, we can segfault.
>
> This is hard to trigger in practice, since we check that the
> path is accessible before creating the alternate_object_database
> struct. But it could be removed racily, or we could see a
> transient filesystem error.
>
> We could restore the original behavior by switching back to
> xstrdup(real_path()).  However, dying is probably not the
> best option here. This whole function is best-effort
> already; there might not even be a repository around the
> shared objects at all. And if the alternate store has gone
> away, there are no objects to show.
>
> So let's just quietly return, as we would if we failed to
> open "refs/", or if upload-pack failed to start, etc.

That's sensible, methinks.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  transport.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/transport.c b/transport.c
> index c86ba2eb8..74d0e45bd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1215,6 +1215,8 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
>  	struct alternate_refs_data *cb = data;
>  
>  	other = real_pathdup(e->path);
> +	if (!other)
> +		return 0;
>  	len = strlen(other);
>  
>  	while (other[len-1] == '/')