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 errors there is a
wasted cycles cloning.
* 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 bad URL display throughout
re-written request handling and when the clone() was corrected to use
urlCanonical() caused asserts in the server-side.
This canonical change has been discussed previously. There is one
other incorrect setting of HttpRequest::canonical in icap/ModXact.cc,
but that must handle a const input which urlCanonical() function does
not yet handle.
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/HttpRequest.cc'
--- src/HttpRequest.cc 2011-05-18 12:50:48 +0000
+++ src/HttpRequest.cc 2011-05-18 13:51:23 +0000
@@ -200,7 +200,7 @@
copy->port = port;
// urlPath handled in ctor
- copy->canonical = canonical ? xstrdup(canonical) : NULL;
+ urlCanonical(copy); // does a re-build from the above copied details.
// range handled in hdrCacheInit()
copy->ims = ims;
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2011-05-11 12:29:30 +0000
+++ src/client_side_request.cc 2011-05-18 14:15:58 +0000
@@ -1042,38 +1042,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.
+ if (!(new_request = urlParse(old_request->method, result, old_request->clone())))
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->flags = old_request->flags;
+ debugs(61,2, HERE << "URL-rewriter diverts URL from " << urlCanonical(old_request) << " to " << urlCanonical(new_request));
+
+ // XXX: why does this not get copied by clone()?
+ new_request->content_length = old_request->content_length;
+
+ // update the new request to flag the re-writing was done on it
new_request->flags.redirected = 1;
-#if USE_AUTH
- new_request->auth_user_request = old_request->auth_user_request;
-#endif
+
+ // unlink bodypipe from the old request. Not needed there any longer.
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;
+ // update the current working ClientHttpRequest fields
+ safe_free(http->uri);
+ http->uri = xstrdup(urlCanonical(new_request));
HTTPMSGUNLOCK(old_request);
http->request = HTTPMSGLOCK(new_request);
}
=== modified file 'src/url.cc'
--- src/url.cc 2011-04-10 02:00:38 +0000
+++ src/url.cc 2011-05-18 14:17:31 +0000
@@ -217,6 +217,9 @@
char *dst;
proto[0] = host[0] = urlpath[0] = login[0] = '\0';
+ if (request)
+ safe_free(request->canonical);
+
if ((l = strlen(url)) + Config.appendDomainLen > (MAX_URL - 1)) {
/* terminate so it doesn't overflow other buffers */
*(url + (MAX_URL >> 1)) = '\0';
