Re: [RFC/PATCH 2/6] transport_get(): Don't SEGFAULT on missing url

Johan Herland [off-list ref] writes:

Signed-off-by: Johan Herland <redacted>

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.

On Thursday 13 August 2009, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

Signed-off-by: Johan Herland <redacted>

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, [off-list ref]
www.herland.net