On 9/06/2012 2:52 a.m., Alex Rousskov wrote:
On 06/08/2012 06:00 AM, Amos Jeffries wrote:On 8/06/2012 9:23 p.m., Alexander Komyagin wrote:Hello Alex!On Thu, 2012-06-07 at 17:22 -0600, Alex Rousskov wrote:On 06/07/2012 10:02 AM, Alexander Komyagin wrote:Hi! I've found that Squid (as of 3.2.0.16) is still overwriting clients requests to HTTP if they are intercepted: src/client_side.cc:prepareTransparentURL(): snprintf(http->uri, url_sz, "http://%s%s";, /*conn->port->protocol,*/ host, url); When I want to intercept https traffic this one really breaks the things down. I have not deepened into the details, but it seems that everything works fine (simultaneous http/https) when I use conn->port->protocol and force protocol to http if it is NULL. Patch is attached.Hello Alexander, It looks like your patch is changing more than just the lines that format the URL. We made a similar but more compact change for the bump-server-first project:static void prepareTransparentURL(ConnStateData * conn, ClientHttpRequest *http, char *url, const char *req_hdr) { char *host; char ipbuf[MAX_IPSTRLEN]; if (*url != '/') return; /* already in good shape */ /* BUG: Squid cannot deal with '*' URLs (RFC2616 5.1.2) */ // BUG 2976: Squid only accepts intercepted HTTP. if ((host = mime_get_header(req_hdr, "Host")) != NULL) { int url_sz = strlen(url) + 32 + Config.appendDomainLen + strlen(host); http->uri = (char *)xcalloc(url_sz, 1); - snprintf(http->uri, url_sz, "http://%s%s";, /*conn->port->protocol,*/ host, url); + snprintf(http->uri, url_sz, "%s://%s%s", conn->port->protocol, host, url); debugs(33, 5, "TRANSPARENT HOST REWRITE: '"<< http->uri<<"'"); } else { /* Put the local socket IP address as the hostname. */ int url_sz = strlen(url) + 32 + Config.appendDomainLen; http->uri = (char *)xcalloc(url_sz, 1); http->getConn()->clientConnection->local.ToHostname(ipbuf,MAX_IPSTRLEN), - snprintf(http->uri, url_sz, "http://%s:%d%s";, - // http->getConn()->port->protocol, + snprintf(http->uri, url_sz, "%s://%s:%d%s", + http->getConn()->port->protocol, ipbuf, http->getConn()->clientConnection->local.GetPort(), url); debugs(33, 5, "TRANSPARENT REWRITE: '"<< http->uri<< "'"); } }Have you seen the port protocol being NULL? It feels like that should not be possible. If it is never NULL, I think the above changes are preferred.Nope. Never seen. But the guy who reported *BUG 2976* certainly did. See http://www.squid-cache.org/mail-archive/squid-dev/201103/0020.htmlAs did several other people around that period. Enough to make me add that workaround to stable releases. It happens whenever Squid is reconfigured. The "conn->port" is a pointer to the SquidConfig http(s)_port state which gets erasesd and rebuilt, problem is the object is reallocated in a different location and the old object being pointed to by this fixed pointer has its string unset. NOTE: when MemPools is disabled this is an invalid pointer dereference crash rather than a silent "NULL". The easiest proper fix is to copy the http_port protocol= string into ConnStateData member for use. ie conn->transportProtocol. The harder one I was working towards is to RefCount the new AnyP::PortCfg objects at which point they will persist as long as that ConnStateData needs to use them. Which is tricky since they are apparently CBDATA which is not compatible with RefCounting.If they are cbdata, let's just lock/release them properly, without ever freeing/deleting them explicitly. My understanding is that the following is necessary to fix the underlying problem: 1. add_http_port() needs to lock the new port pointer before linking it. 2. parsePortCfg() needs to lock the new port pointer before linking it. 3. free_PortCfg() needs to unlock the old port pointer before unlinking it. It should not delete the old pointer. Anything else?
ConnStateData already seems to set a reference on the details. So yes, that looks like the quickest fix.
Amos
