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.
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
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2011-05-19 12:02:58 +0000
+++ src/client_side_request.cc 2011-05-19 12:50:57 +0000
@@ -1016,7 +1016,6 @@
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);
@@ -1042,41 +1041,33 @@
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 (!(new_request = urlParse(old_request->method, result, new_request))) {
debugs(85, DBG_CRITICAL, "ERROR: URL-rewrite produces invalid request: " <<
old_request->method << " " << result << " HTTP/1.1");
- }
- }
-
- 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->clientConnectionManager = old_request->clientConnectionManager;
- new_request->flags = old_request->flags;
- new_request->flags.redirected = 1;
-#if USE_AUTH
- new_request->auth_user_request = old_request->auth_user_request;
-#endif
- 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);
+ delete new_request;
+ } else {
+ 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);
+ }
+ }
}
/* FIXME PIPELINE: This is innacurate during pipelining */
=== modified file 'src/url.cc'
--- src/url.cc 2011-04-10 02:00:38 +0000
+++ src/url.cc 2011-05-19 13:22:43 +0000
@@ -46,7 +46,7 @@
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"
@@ -236,7 +236,7 @@
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;
@@ -442,6 +442,7 @@
request = new HttpRequest(method, protocol, urlpath);
else {
request->initHTTP(method, protocol, urlpath);
+ safe_free(request->canonical);
}
request->SetHost(host);
@@ -451,9 +452,15 @@
}
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, AnyP::PROTO_URN, urn + 4);
+ safe_free(request->canonical);
+ return request;
+ }
+
return new HttpRequest(method, AnyP::PROTO_URN, urn + 4);
}
