Re: [PATCH] URL handling bugs

On 19/05/11 03:50, Alex Rousskov wrote:
> On 05/18/2011 09:36 AM, Alex Rousskov wrote:
>> On 05/18/2011 08:52 AM, Amos Jeffries wrote:
>>> This patch includes two bug fixes in URL handling which were uncovered
>>> during testing of the URL logging update:
>>>
>>> * URL re-write handling was not correctly creating its adapted request
>>> copy. The code here is much reduced by using the clone() method. Still
>>> not completely satisfactory (marked with XXX) since on errors there is a
>>> wasted cycles cloning.
>>
>> Wasting a few cycles on errors would be OK, I guess, but there is also
>> at least a possibility of a memory leak: HttpRequest::clone() always
>> produces a new request, but AFAICT urlParse() sometimes returns NULL
>> instead of the request, without deleting the request parameter.
>
> Even worse, if the new url is a urn, parseUrl returns a brand new request:
>
>> } else if (!strncmp(url, "urn:", 4)) {
>> return urnParse(method, url);
>
> which means that our attempt to use a cloned request failed _and_ we now
> have two requests: the one created by urnParse(), which lacks clone
> information, and the one created by clone(), which lacks the new URL.
>
> I do not know whether anybody still cares about URNs these days, but
> this code path is not going to work well.

Arg. What a catch.

>
> The core problem here may be in trying to combine two different
> interfaces into one:
>
> 1. Trying to give an old request a new method/URL. This should be done
> inside HttpRequest::resetUrl() or similar. This will not create new
> requests or delete old ones.
>
> 2. Trying to create a new request using a method/URL pair. This can be
> done as a stand-alone function or a static HttpRequest method that
> returns a new request. This function should not have a request as a
> parameter.
>

Yes. Pretty much what I've done to detach them in the test branch. Looks
like I'll have to push that forward a bit further.

As a workaround I'll look at making urlParse and urnParse have the same
API pass-thru behaviour.

Thanks.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.7 and 3.1.12.1