> On Thu, 2008-04-10 at 00:56 +1200, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>> > On Tue, 2008-04-08 at 17:41 +0900, S.KOBAYASHI wrote:
>> >
>> >> I'm handling ipv6 functions on ICAP, so I found a curious point.
>> >> ICAP capable is ON and use ipv6 URI with SSL from the browser such as
>> >> https://[ 2001:5c0:9a37:0:21d:9ff:fe68:333a]/.
>> >> CONNECT header in the ICAP request-line which was sent from squid to
>> ICAP
>> >> server is "0.0.7.209:443 HTTP1.1".
>> >> I think it should be included ipv6 address, not "0.0.7.209".
>> >> I'm using squid version Squid-3.HEAD-20080320.
>> >
>> > Please file a "IPv6 IPs in ICAP URLs" bug report with bugzilla,
>> > especially if nobody sends you a fix for this bug here.
>> >
>> > Thank you,
>> >
>> > Alex.
>> >
>>
>> (Moving to Squid-dev as this should be discussed there.)
>>
>> Alex, is ICAP not using the built-in squid URL Parser and IPAddress to
>> handle URLs?
>
> It does, but I believe this is about request generation, not parsing.
> The generation code uses "built-in" HttpMsg::packInto:
>
>> void ICAPModXact::packHead(MemBuf &httpBuf, const HttpMsg *head)
>> {
>> Packer p;
>> packerToMemInit(&p, &httpBuf);
>> head->packInto(&p, true);
>> packerClean(&p);
>> }
>
> So, either the bug is in core packInto or ICAP encapsulation code that
> calls it. The latter is not trivial because the core does not support
> true HTTP message header cloning. We have to do it ourselves to remove
> hop-by-hop headers from the virgin HTTP header:
>
>> HttpRequest* new_request = new HttpRequest;
>> urlParse(old_request->method, old_request->canonical, new_request);
>> new_request->http_ver = old_request->http_ver;
>> inheritVirginProperties(*new_request, *old_request);
>> headClone = new_request;
>>
>> ... clone all header fields using HttpHeaderEntry::clone()
>>
>> headClone->header.removeHopByHopEntries();
>> packHead(httpBuf, headClone);
>
> So, if packInto is not buggy, then either urlParse is or the above
> cloning code is missing something. FWIW, the HTTP Request line is
> produced by HttpRequest::packableURI(true), which calls urlCanonical(),
> which does this for CONNECT requests:
>
>> case METHOD_CONNECT:
>> snprintf(urlbuf, MAX_URL, "%s:%d", request->GetHost(),
>> request->port);
>> break;
>
> So, GetHost() apparently returns garbage. As far as I can tell, the host
> member returned by GetHost is set by urlParse(). I would start by adding
> a Must() around urlParse call above and then figuring out why it does
> not set host properly for CONNECT.
GetHost() is fairly well unit-tested so its can only fail on GIGO cases. I
think I have found it in the CONNECT branch of urlParse().
I think ideally there should not be a seperate CONNECT branch where it is,
but we can test that separately to the bug fix.
Amos
