From: Johan Herland <johan@herland.net>
Date: Wed, 12 Aug 2009 23:39:10 GMT
Subject: Re: [RFC/PATCH 2/6] transport_get(): Don't SEGFAULT on missing url
Message-ID: <200908130139.10876.johan@herland.net>
In-Reply-To: <7vab24wtzo.fsf@alter.siamese.dyndns.org>

On Thursday 13 August 2009, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > Signed-off-by: Johan Herland <johan@herland.net>
>
> How does url end up to be NULL?  At the beginning of transport_get(), you
> do this:
>
> 	ret->remote = remote;
> 	if (!url && remote && remote->url)
> 		url = remote->url[0];
> 	ret->url = url;
> 	if (remote && remote->foreign_vcs) {
> 		transport_helper_init(ret);
> 		return ret;
> 	}
>
> and the case where remote.$name.vcs is defined, we do not need
> remote.$name.url.
>
> When (!url && remote && remote->url), is remote->url[0] allowed to be
> NULL?  I am guessing it would be a bug in whoever prepared the remote,
> and if that is indeed the case, the patch shifts the symptoms without
> fixing the cause.
>
> When (remote && remote->foreign_vcs) does not hold, iow, if no remote is
> defined or the remote is defined but lacks remote.$name.url, you will go
> to the last else clause in the function that sets up a git_transport_data
> for the native transport, but it has ret->url == NULL.
>
> Whom does that transport talk with?  Is such a transport of any use, or
> does it cause a segfault downstream in the call chain?
>
> In other words, I am wondering if this patch should just diagnose the
> case as an error, instead of pretending all is well.

You are right. Instead of pushing the segfault downstream, we should verify 
that the url is non-NULL before returning it as part of ret (modulo the 
foreign case, of course).


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net