Re: [PATCH] URL handling bugs

On 05/20/2011 12:09 AM, Amos Jeffries wrote:
> 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.

clone() should guarantee a non-NULL result, IMO. In the worst case (lack
of RAM, etc) it should throw, but never return NULL.

> Commit time?

Sure, if you are happy with the patch.

Thank you,

Alex.
Received on Fri May 20 2011 - 17:32:09 MDT