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.html > > As 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? Thank you, Alex.
