Re: [PATCH] URL handling bugs

On 20/05/11 03:37, Alex Rousskov wrote:
> On 05/19/2011 08:07 AM, Amos Jeffries wrote:
>> UPDATE: just the updated clone fix and minimal url.cc changes it depends
>> on. canonical handling cleanups are coming later.
>>
>>
>> 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 invalid URL there
>> is a wasted cycles cloning and deleting almost immediately. Future
>> cleanups moving the URL parts outside HttpRequest will fix that.
>>
>> * 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 incorrect canonical URL to be
>> used following re-write. When the cloning above was corrected it caused
>> asserts in the server-side.
>>
>> * To prevent memory leaks the urnParse() function internal to URL
>> parsing is adjusted to accept and update an existing request in
>> identical API semantics to urlParse() instead of always generating a new
>> one.
>
>> === modified file 'src/client_side_request.cc'
>> + HttpRequest *new_request = old_request->clone();
>> + if (!(new_request = urlParse(old_request->method, result, new_request))) {
>
>
> This is not a bug anymore, but I would still either remove the
> new_request assignment inside the if-statement (to remove the
> implication that new_request may change there) OR assign it to a
> different variable and assert that both are the same:
>
> A:
>
> HttpRequest *new_request = old_request->clone();
> if (urlParse(old_request->method, result, new_request))
> ...
>

Changed. This is acceptable for now, given that this will all change
again when URL class is polished up.
  Meanwhile if clone() or its rename ever produces NULL it will leak for
the same reasons urnParse() used to.

Commit time?

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