Re: [PATCH] Squid host rewrite for intercepted https requests

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.
Received on Fri Jun 08 2012 - 14:52:22 MDT