From 47cbd376b845d8bf424625e5ec42f46b1e757b24 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sat, 28 May 2011 22:40:52 -0600 Subject: [PATCH] URL re-writer handling bug fixes 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. --- src/client_side_request.cc | 59 ++++++++++++++++---------------------- src/url.cc | 14 +++++++-- 2 files changed, 35 insertions(+), 38 deletions(-) diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 5f3d35ed2c..85a715d3bf 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1000,7 +1000,6 @@ clientRedirectDoneWrapper(void *data, char *result) void ClientRequestContext::clientRedirectDone(char *result) { - HttpRequest *new_request = NULL; HttpRequest *old_request = http->request; debugs(85, 5, "clientRedirectDone: '" << http->uri << "' result=" << (result ? result : "NULL")); assert(redirect_state == REDIRECT_PENDING); @@ -1026,45 +1025,35 @@ ClientRequestContext::clientRedirectDone(char *result) debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid 303 redirect Location: " << result); } } else if (strcmp(result, http->uri)) { - if (!(new_request = HttpRequest::CreateFromUrlAndMethod(result, old_request->method))) + // XXX: validate the URL properly *without* generating a whole new request object right here. + // XXX: the clone() should be done only AFTER we know the new URL is valid. + HttpRequest *new_request = old_request->clone(); + if (urlParse(old_request->method, result, new_request)) { + debugs(61,2, HERE << "URL-rewriter diverts URL from " << urlCanonical(old_request) << " to " << urlCanonical(new_request)); + + // update the new request to flag the re-writing was done on it + new_request->flags.redirected = 1; + + // unlink bodypipe from the old request. Not needed there any longer. + if (old_request->body_pipe != NULL) { + old_request->body_pipe = NULL; + debugs(61,2, HERE << "URL-rewriter diverts body_pipe " << new_request->body_pipe << + " from request " << old_request << " to " << new_request); + } + + // update the current working ClientHttpRequest fields + safe_free(http->uri); + http->uri = xstrdup(urlCanonical(new_request)); + HTTPMSGUNLOCK(old_request); + http->request = HTTPMSGLOCK(new_request); + } else { debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid request: " << old_request->method << " " << result << " HTTP/1.1"); + delete new_request; + } } } - if (new_request) { - safe_free(http->uri); - http->uri = xstrdup(urlCanonical(new_request)); - new_request->http_ver = old_request->http_ver; - new_request->header.append(&old_request->header); - new_request->client_addr = old_request->client_addr; -#if FOLLOW_X_FORWARDED_FOR - new_request->indirect_client_addr = old_request->indirect_client_addr; -#endif /* FOLLOW_X_FORWARDED_FOR */ - new_request->my_addr = old_request->my_addr; - new_request->flags = old_request->flags; - new_request->flags.redirected = 1; - - if (old_request->auth_user_request) { - new_request->auth_user_request = old_request->auth_user_request; - AUTHUSERREQUESTLOCK(new_request->auth_user_request, "new request"); - } - - if (old_request->body_pipe != NULL) { - new_request->body_pipe = old_request->body_pipe; - old_request->body_pipe = NULL; - debugs(61,2, HERE << "URL-rewriter diverts body_pipe " << new_request->body_pipe << - " from request " << old_request << " to " << new_request); - } - - new_request->content_length = old_request->content_length; - new_request->extacl_user = old_request->extacl_user; - new_request->extacl_passwd = old_request->extacl_passwd; - new_request->flags.proxy_keepalive = old_request->flags.proxy_keepalive; - HTTPMSGUNLOCK(old_request); - http->request = HTTPMSGLOCK(new_request); - } - /* FIXME PIPELINE: This is innacurate during pipelining */ if (http->getConn() != NULL) diff --git a/src/url.cc b/src/url.cc index 54252af9d8..c6dcd8abfd 100644 --- a/src/url.cc +++ b/src/url.cc @@ -45,7 +45,7 @@ static HttpRequest *urlParseFinish(const HttpRequestMethod& method, const char *const login, const int port, HttpRequest *request); -static HttpRequest *urnParse(const HttpRequestMethod& method, char *urn); +static HttpRequest *urnParse(const HttpRequestMethod& method, char *urn, HttpRequest *request); static const char valid_hostname_chars_u[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" @@ -235,7 +235,7 @@ urlParse(const HttpRequestMethod& method, char *url, HttpRequest *request) port = urlDefaultPort(protocol); return urlParseFinish(method, protocol, url, host, login, port, request); } else if (!strncmp(url, "urn:", 4)) { - return urnParse(method, url); + return urnParse(method, url, request); } else { /* Parse the URL: */ src = url; @@ -441,6 +441,7 @@ urlParseFinish(const HttpRequestMethod& method, request = new HttpRequest(method, protocol, urlpath); else { request->initHTTP(method, protocol, urlpath); + safe_free(request->canonical); } request->SetHost(host); @@ -450,9 +451,16 @@ urlParseFinish(const HttpRequestMethod& method, } static HttpRequest * -urnParse(const HttpRequestMethod& method, char *urn) +urnParse(const HttpRequestMethod& method, char *urn, HttpRequest *request) { debugs(50, 5, "urnParse: " << urn); + + if (request) { + request->initHTTP(method, PROTO_URN, urn + 4); + safe_free(request->canonical); + return request; + } + return new HttpRequest(method, PROTO_URN, urn + 4); } -- 2.47.2