Re: [PATCH] URL handling bugs

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))
    ...

B:

    HttpRequest *new_request = old_request->clone();
    if (HttpRequest *r2 = urlParse(old_request->method, result,
new_request))
        assert(r2 == new_request);

I do not see any problems with the updated patch.

Thank you,

Alex.
Received on Thu May 19 2011 - 15:38:19 MDT