Here is a patch which implements Alex proposal.
Regards,
Christos
On 06/08/2012 05:52 PM, 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.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.
>
>
>
>
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2012-05-24 00:23:52 +0000
+++ src/cache_cf.cc 2012-06-13 16:23:15 +0000
@@ -3752,84 +3752,85 @@
s->generateHostCertificates = true;
} else if (strcmp(token, "generate-host-certificates=on") == 0) {
s->generateHostCertificates = true;
} else if (strcmp(token, "generate-host-certificates=off") == 0) {
s->generateHostCertificates = false;
} else if (strncmp(token, "dynamic_cert_mem_cache_size=", 28) == 0) {
parseBytesOptionValue(&s->dynamicCertMemCacheSize, B_BYTES_STR, token + 28);
#endif
} else {
self_destruct();
}
}
void
add_http_port(char *portspec)
{
AnyP::PortCfg *s = new AnyP::PortCfg("http_port");
parsePortSpecification(s, portspec);
// we may need to merge better if the above returns a list with clones
assert(s->next == NULL);
- s->next = Config.Sockaddr.http;
- Config.Sockaddr.http = s;
+ s->next = cbdataReference(Config.Sockaddr.http);
+ cbdataReferenceDone(Config.Sockaddr.http);
+ Config.Sockaddr.http = cbdataReference(s);
}
static void
parsePortCfg(AnyP::PortCfg ** head, const char *optionName)
{
const char *protocol = NULL;
if (strcmp(optionName, "http_port") == 0 ||
strcmp(optionName, "ascii_port") == 0)
protocol = "http";
else if (strcmp(optionName, "https_port") == 0)
protocol = "https";
if (!protocol) {
self_destruct();
return;
}
char *token = strtok(NULL, w_space);
if (!token) {
self_destruct();
return;
}
AnyP::PortCfg *s = new AnyP::PortCfg(protocol);
parsePortSpecification(s, token);
/* parse options ... */
while ((token = strtok(NULL, w_space))) {
parse_port_option(s, token);
}
if (Ip::EnableIpv6&IPV6_SPECIAL_SPLITSTACK && s->s.IsAnyAddr()) {
// clone the port options from *s to *(s->next)
- s->next = s->clone();
+ s->next = cbdataReference(s->clone());
s->next->s.SetIPv4();
debugs(3, 3, protocol << "_port: clone wildcard address for split-stack: " << s->s << " and " << s->next->s);
}
while (*head)
head = &(*head)->next;
- *head = s;
+ *head = cbdataReference(s);
}
static void
dump_generic_port(StoreEntry * e, const char *n, const AnyP::PortCfg * s)
{
char buf[MAX_IPSTRLEN];
storeAppendPrintf(e, "%s %s",
n,
s->s.ToURL(buf,MAX_IPSTRLEN));
// MODES and specific sub-options.
if (s->intercepted)
storeAppendPrintf(e, " intercept");
else if (s->spoof_client_ip)
storeAppendPrintf(e, " tproxy");
else if (s->accel) {
storeAppendPrintf(e, " accel");
@@ -3938,41 +3939,41 @@
#endif
}
static void
dump_PortCfg(StoreEntry * e, const char *n, const AnyP::PortCfg * s)
{
while (s) {
dump_generic_port(e, n, s);
storeAppendPrintf(e, "\n");
s = s->next;
}
}
static void
free_PortCfg(AnyP::PortCfg ** head)
{
AnyP::PortCfg *s;
while ((s = *head) != NULL) {
*head = s->next;
- delete s;
+ cbdataReferenceDone(s);
}
}
void
configFreeMemory(void)
{
free_all();
#if USE_SSL
SSL_CTX_free(Config.ssl_client.sslContext);
#endif
}
void
requirePathnameExists(const char *name, const char *path)
{
struct stat sb;
char pathbuf[BUFSIZ];
assert(path != NULL);
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2012-04-25 05:29:20 +0000
+++ src/client_side.cc 2012-06-13 20:51:00 +0000
@@ -2076,49 +2076,49 @@
debugs(33, 5, "ACCEL VPORT REWRITE: '" << http->uri << "'");
}
}
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 << "'");
}
}
/**
* parseHttpRequest()
*
* Returns
* NULL on incomplete requests
* a ClientSocketContext structure on success or failure.
* Sets result->flags.parsed_ok to 0 if failed to parse the request.
* Sets result->flags.parsed_ok to 1 if we have a good request.
*/
static ClientSocketContext *
parseHttpRequest(ConnStateData *csd, HttpParser *hp, HttpRequestMethod * method_p, HttpVersion *http_ver)
{
char *req_hdr = NULL;
char *end;
size_t req_sz;
