Re: [PATCH] URL handling bugs

On 19/05/11 03:36, 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. Thus, we
> must not do the following:
>
> urlParse(..., old_request->clone())
>
> but have to do something like
>
> new_request = old_request->clone();
> if (!urlParse(new_request->method, result, new_request))) {
> debugs(...);
> delete new_request;
> new_request = NULL;
> }
>
> which still looks terrible but at least avoids memory leaks.

Okay thanks. Done.

>
> Eventually, this would become an HttpRequest::resetUrl() or reparseUrl()
> method, I guess:
>
> new_request = old_request->clone();
> if (!new_request->resetUrl(result)) {
> ...
> }
>
> (assuming there are good reasons for urlParse() to require access to
> request details), but such changes are probably outside your patch scope.

There is no good reason. I have a hobby branch working away at that
separation slowly.

>
>
>> + // XXX: why does this not get copied by clone()?
>> + new_request->content_length = old_request->content_length;
>
> Because it is set in HttpHdr::hdrCacheInit() called by
> HttpRequest::clone(). You should be able to remove the above two lines.

Sweet! done.

>
> It may be more efficient to add HttpHdr::clone() instead of initializing
> its cache from scratch, of course, but that optimization is probably
> outside your patch scope.

Yes.

>
>
>> - copy->canonical = canonical ? xstrdup(canonical) : NULL;
>> + urlCanonical(copy); // does a re-build from the above copied details.
>
> This change seems like a step backwards to me. We should be able to just
> clone the canonical field when cloning the request. Why do we need to
> rebuild it from scratch? The clone() method has the right to correctly
> clone low-level bits. We should treat it as a copy constructor,
> essentially (in fact, it should probably be eventually implemented using
> a private copy constructor).
>
> If the canonical field of the being-cloned request is not valid (and
> hence should not be cloned), the bug is elsewhere.
>
> The urlCanonical(request) global should be converted into a constant
> HttpRequest method, but that may be outside your patch scope. In any
> case, urlCanonical() is not designed for cloning and, unlike the HttpHdr
> cache discussed above, we already have a simple xstrdup() cloning
> solution here that we should use.
>

Hmm. Okay. In the name of optimization. Reverted.

>
>> * URL parsing needs to set the canonical field to unset whenever the
>> URI is re-parsed into a request. This field is an optimization for later
>> display speed-ups. This has been causing bad URL display throughout
>> re-written request handling
>
> Can you rephrase using a word other than "display"? I think I agree with
> the above, but it is not clear what you mean by "display". Perhaps you
> can say that stale canonical field values were used throughout Squid
> after a request was rewritten?

Okay. Will do.

>> This canonical change has been discussed previously. There is one other
>> incorrect setting of HttpRequest::canonical in icap/ModXact.cc, but that
>> must handle a const input which urlCanonical() function does not yet
>> handle.
>
> To fix that, urlCanonical(request) should become a constant HttpRequest
> method. Is there an XXX about this in icap/ModXact.cc? If not, please add.

I'm not so sure. Making it constant voids the caching speed benefit. The
existing xstrdup() is fine in the name of speed.

Thank you for the quick review.

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