Re: disable-loadable-modules
On Thu, 2009-07-09 at 20:22 +1200, Amos Jeffries wrote: Robert Collins wrote: On Thu, 2009-07-09 at 19:40 +1200, Amos Jeffries wrote: ... checking whether to use loadable modules... yes, implicitly Is the test for whether to use them broken? The code is to always enables them by default unless forced not to. Everything would work just fine if libtool would only add the $(builddir) to the path its trying to locate locally built things on. It will if its been told that its using local libraries. I suspect we're misusing something in libtool/automake to generate these symptoms. -Rob signature.asc Description: This is a digitally signed message part
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
Alex Rousskov wrote: On 07/09/2009 12:06 AM, Amos Jeffries wrote: Tsantilas Christos wrote: This is the second part of the 3p1-plus patch. This patch include: - Enhanced access logging (Hs, sh, sh, pt, tt, icap::total_time, and icap::last_h) - ICAP logging (see icap_log and log_icap in squid.conf as well as http://wiki.squid-cache.org/Features/AdaptationLog). - ICAP retries based on the ICAP responses status code (see icap_retry and icap_retry_limit in squid.conf). - Support logging resp. times of adaptation transactions to access.log (%adaptation::sum_trs and %adaptation::all_trs) IMO full 'adaptation' is too long. Opinions on using the abbreviation 'adapt::' here? Either way is fine with me. If Christos does not prefer the full version, let's change to adapt:: - Support logging of total DNS wait time to access.log (%dt) +1. The only sightly better but harder way is to make this DnsLookupDetails object the DNS cbdata object itself, and contain the RR being fetched as a sub-field. That would keep the error per-fetch not per transaction. If someone wants to simplify the code later theres a project. Meanwhile go with this. I think I have started with something similar but then realized that different parts of the code (e.g., IP cache and FQDN cache) need different objects to store the actual lookup result. They also use pointers into caches that may not survive across async calls (because the cache may change) so storing the result would require allocating memory or locking cache entries for a long time. At some point, I made DnsLookupResult store and then forget the RR-level details when crossing the async boundary. That code looked somewhat over-engineered to me. This is why I eventually limited the DnsLookupDetails object to the details that need to cross the async call boundaries and are common to all lookup users. I do not mean to discourage others from trying to add RR or derivative results to DnsLookupDetails. Just keep the above in mind. - Bug #2495 fix Oops; bug is: 2459: dns_error_message global does not work... My fault. I had it backwards in some of the notes that Christos inherited. ** Please make fqdncache_entry / ipcache_entry public classes instead of functional structs. Some gcc complain and doxygen won't document their functions properly. Sorry, I am not sure what you mean. What is a functional struct? Do you want us to replace struct fqdncache_entry with class fqdncache_entry and add public:? Yes exactly. No objections, of course, just wanted to minimize merging headaches by keeping out-of-scope changes to the minimum... I also have some questions to developers: - In squid.conf documentation I prepend each HTTP format code with [http::] Is it OK? Most of them yes. There are a few where http:: does not make sense at all: tsSeconds since epoch tusubsecond time (milliseconds) tlLocal time. Optional strftime format argument tgGMT time. Optional strftime format argument Definitely. dtTotal time spent making DNS lookups (milliseconds) uiUser name from ident usUser name from SSL Yeah, at least for now. These are also borderline: aClient source IP address AClient FQDN pClient source port laLocal IP address (http_port) lpLocal port number (http_port) AServer IP address or peer name Agreed. I'm fine with ignoring http:: for now if people do enter it for these, so no coding change. But I don't think we should document them that way. Agreed. I'd shift the clearly HTTP ones out to a section like you have ICAP. Leaving the rest prefix-ambiguous until something is done properly. Or at least remove the http:: prefix in the documentation from those mentioned above. That makes a bit of a mess in the list with mixing prefixed and non-prefixed. I think the sections look good and are easily understandable the way Christos did the icap:: and adaptation:: - Should we replace icap::Hs with icap::Hs ? Any opinion? (My opinion is we should not because the icap::Hs does not make sense) Agreed. Makes no sense at this time. OK. In src/AccessLogEntry.h: struct IpAddress = remove the struct, it's a class in 3.1+. OK. Where an existing short measure tag I don't think we need to add a new long one yet: icap::total_time / icap::tt icap::size / icap::st icap::size / icap::st OK. adaptation::sum_trs / adaptation::tt These are very different! Tt is a single value while adapt::sum_trs is a list (of individual tr's). Adapt::sum_trs is also symmetric with adapt::all_trs (also a list, but possibly with more entries). You sure? I spent a while trying to figure out what all_trs and sum_trs were and it looked like all_trs was the list and sum_trs was the single-value sum of them. If I did get it wrong, then the tt reduction still applies, but to the other
Re: about https support for transparent proxy
Alex Rousskov wrote: On 07/03/2009 08:48 AM, Mikio Kishi wrote: Hi, Alex and Henrik I'm sorry, there is a lack of explanation It looks like you are working on a useful feature, but can you explain in more detail what your patch does? Why is the feature called SslConnect? Is it specific to tproxy environments or can it work with any transparent Squid? Does it work in combination with SslBump or are they mutually exclusive? - motivation http://wiki.squid-cache.org/Features/Tproxy4 The above is still not supported https. I'd like to support https. In addition to above, the following configuration can support https with my patch. - squid configuration http_port 8443 tproxy sslConnect - iptables configuration iptables -t mangle -A PREROUTING -p tcp --dport 443 -j TPROXY --tproxy-mark 0x1/0x1 --on-port 8443 Do not work with SslBump I think. SslBump requires the CONNECT right? Yes. SslBump is not relate to sslConnect, but I'm interested in SslBump. Please rename sslConnect in squid.conf and in source code to sslTunnel or, if you do not plan on doing anything SSL-specific, to tcpTunnel. Other names may work better than my suggestion, but I think we should avoid the word connect that can be easily confused with HTTP CONNECT requests. Please see my earlier email for other comments. I also agree with Henrik that tcp_port may be a good idea because your code does not work on HTTP level, but I understand that it is a bigger change. Thank you, Alex. I still don't quite see how much of the internal traffic Squid gets access to with this. Please definitely call it sslTunnnel and add some detailed documentation about what the traffic does when inside Squid. ie does it get igored and shoved back out again at eariest convenience, or passed through all of Squids normal http_port traffic controls, shaping, rewriting and forwarding mechanisms? Also Please correct this: +// set url +int url_sz = 32 + Config.appendDomainLen; +http-uri = (char *)xcalloc(url_sz, 1); +snprintf(http-uri, url_sz, %s:%d, + http-getConn()-me.NtoA(ntoabuf,MAX_IPSTRLEN), + http-getConn()-me.GetPort()); MAX_IPSTRLEN is greater then 32 all by itself. Things go badly wrong when Config.appendDomainLen is less than the difference. Also the IpAddress ToURL() call does correct address:port creation for use in URLs like this. Please use: http-uri = (char *)xcalloc(MAX_IPSTRLEN, 1); http-getConn()-me.ToURL(http-uri,MAX_IPSTRLEN) I guess it could be extended to respond with an SSL level error notification in these cases, but not sure it's worth the effort. Right. I think that just comm_close() is simple... To be honest, https_port 8443 tproxy sslConnect is better. But it's easier to hack http_port handling than https_port. What do you think of my patch ? Sincerely, -- Mikio Kishi On Tue, Jun 30, 2009 at 6:31 AM, Alex Rousskovrouss...@measurement-factory.com wrote: On 06/29/2009 01:32 PM, Henrik Nordstrom wrote: sön 2009-06-28 klockan 14:18 -0600 skrev Alex Rousskov: Ok, but can you tell what the patch does? Forwards raw SSL connections to the next hop, as if Squid was a TCP proxy? Yes. Something else? Not really. But supports both forwarded mode and standalone (connecting direct, or via a parent proxy). OK, I see. Do not work with SslBump I think. SslBump requires the CONNECT right? I do not think so. In my tests, SslBump worked for WCCP-intercepted SSL connections. Are you sure that's SslBump, and not just https_port? You may be right. I thought I did change something in https_port when working on SslBump but I may be misremembering. If I did, it was certainly very little because most of the work was on the CONNECT requests handling. I do remember that I tested WCCP redirection of port 443 traffic and it worked (with certificate warnings). https_port works kind of in interception mode, if the certificate warnings/errors is ignored.. has always been like that just not documented very well. Agreed. Note: SslBump (long term) could be made to work in interception mode with modern browsers sending the requested hostname in the initial SSL hello message. Thank you, Alex. Amos -- Please be using Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16 Current Beta Squid 3.1.0.9
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
Alex Rousskov wrote: On 07/09/2009 12:06 AM, Amos Jeffries wrote: Tsantilas Christos wrote: This is the second part of the 3p1-plus patch. This patch include: - Enhanced access logging (Hs, sh, sh, pt, tt, icap::total_time, and icap::last_h) - ICAP logging (see icap_log and log_icap in squid.conf as well as http://wiki.squid-cache.org/Features/AdaptationLog). - ICAP retries based on the ICAP responses status code (see icap_retry and icap_retry_limit in squid.conf). - Support logging resp. times of adaptation transactions to access.log (%adaptation::sum_trs and %adaptation::all_trs) IMO full 'adaptation' is too long. Opinions on using the abbreviation 'adapt::' here? Either way is fine with me. If Christos does not prefer the full version, let's change to adapt:: The adapt:: is better. - Support logging of total DNS wait time to access.log (%dt) +1. The only sightly better but harder way is to make this DnsLookupDetails object the DNS cbdata object itself, and contain the RR being fetched as a sub-field. That would keep the error per-fetch not per transaction. If someone wants to simplify the code later theres a project. Meanwhile go with this. I think I have started with something similar but then realized that different parts of the code (e.g., IP cache and FQDN cache) need different objects to store the actual lookup result. They also use pointers into caches that may not survive across async calls (because the cache may change) so storing the result would require allocating memory or locking cache entries for a long time. At some point, I made DnsLookupResult store and then forget the RR-level details when crossing the async boundary. That code looked somewhat over-engineered to me. This is why I eventually limited the DnsLookupDetails object to the details that need to cross the async call boundaries and are common to all lookup users. I do not mean to discourage others from trying to add RR or derivative results to DnsLookupDetails. Just keep the above in mind. - Bug #2495 fix Oops; bug is: 2459: dns_error_message global does not work... OK My fault. I had it backwards in some of the notes that Christos inherited. ** Please make fqdncache_entry / ipcache_entry public classes instead of functional structs. Some gcc complain and doxygen won't document their functions properly. Sorry, I am not sure what you mean. What is a functional struct? Do you want us to replace struct fqdncache_entry with class fqdncache_entry and add public:? No objections, of course, just wanted to minimize merging headaches by keeping out-of-scope changes to the minimum... I did not touch it. The struct used in many places inside squid3 I also have some questions to developers: - In squid.conf documentation I prepend each HTTP format code with [http::] Is it OK? Most of them yes. There are a few where http:: does not make sense at all: tsSeconds since epoch tusubsecond time (milliseconds) tlLocal time. Optional strftime format argument tgGMT time. Optional strftime format argument Definitely. dtTotal time spent making DNS lookups (milliseconds) uiUser name from ident usUser name from SSL Yeah, at least for now. These are also borderline: aClient source IP address AClient FQDN pClient source port laLocal IP address (http_port) lpLocal port number (http_port) AServer IP address or peer name Agreed. I'm fine with ignoring http:: for now if people do enter it for these, so no coding change. But I don't think we should document them that way. Agreed. I'd shift the clearly HTTP ones out to a section like you have ICAP. Leaving the rest prefix-ambiguous until something is done properly. Or at least remove the http:: prefix in the documentation from those mentioned above. I remove the http:: prefix for the format codes mentioned above. But maybe the use of http:: prefix needs more discussion... - Should we replace icap::Hs with icap::Hs ? Any opinion? (My opinion is we should not because the icap::Hs does not make sense) Agreed. Makes no sense at this time. OK. In src/AccessLogEntry.h: struct IpAddress = remove the struct, it's a class in 3.1+. OK. done Where an existing short measure tag I don't think we need to add a new long one yet: icap::total_time / icap::tt icap::size / icap::st icap::size / icap::st OK done . adaptation::sum_trs / adaptation::tt These are very different! Tt is a single value while adapt::sum_trs is a list (of individual tr's). Adapt::sum_trs is also symmetric with adapt::all_trs (also a list, but possibly with more entries). Yes they are not ideal, but they are there and what people are used to for now. We can minimize confusion a bit. OK. done for total_time and size I think at the point you do: +case
[PATCH] Enhanced access logging, added ICAP logging, bug #2459 fix version2
Changes over the v1 patch: changes: - src/AccessLogEntry.h: struct IpAddress - IpAddress - access log code: * adaptation:: to adapt:: * icap::total_time - icap::tt * icap::size - icap::st * icap::size - icap::st * int the case of use of LFT_HTTP_SENT_STATUS_CODE_OLD_30 convert internaly to LFT_HTTP_SENT_STATUS_CODE - Remove the http:: prefix from documentation for the format codes: a A p A la lp ts tu tl tg tr dt - adaptation/icap/Launcher.cc, adaptation/icap/Xaction.cc: remove the #include acl/Checklist.h Regards, Christos 3p1-plus-mod-v2.patch.gz Description: Unix tar archive
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2459 fix version2
Bundle Buggy has detected this merge request. For details, see: http://bundlebuggy.aaronbentley.com/project/squid/request/%3C4A56EFF5.3020202%40users.sourceforge.net%3E Project: Squid
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
Hi Amos, I post the new patch. I think I made most of the changes you are requested. I did not do the following changes: ** Please make fqdncache_entry / ipcache_entry public classes instead of functional structs. Some gcc complain and doxygen won't document their functions properly. and I spy a goto that can die... case CLF_NONE: goto last; @@ -1458,7 +1863,15 @@ last: (void)0; /* NULL statement for label */ +} == equates to return;. Please check for others in that same function. I have not any special reason and I will do the changes if required, but it is better for now to focus on new features added. Other changes can applied with separate patches I hope it is OK. Regards, Christos
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
Tsantilas Christos wrote: Alex Rousskov wrote: On 07/09/2009 12:06 AM, Amos Jeffries wrote: Tsantilas Christos wrote: ** Please make fqdncache_entry / ipcache_entry public classes instead of functional structs. Some gcc complain and doxygen won't document their functions properly. Sorry, I am not sure what you mean. What is a functional struct? Do you want us to replace struct fqdncache_entry with class fqdncache_entry and add public:? No objections, of course, just wanted to minimize merging headaches by keeping out-of-scope changes to the minimum... I did not touch it. The struct used in many places inside squid3 Please do change. The rest of the code fails to prefix the names with struct the typedef made that possible. Using class and public: is the C++ upgrade equivalent to the typedef and allows functions/methods inside the struct where I suspect the typedef did not. If there are code references to struct fqdncache_entry or struct ipcache_entry, they are well within scope of the change removing relevant typedefs. FWIW: I find none in the existing code. Which makes the change safe. I also have some questions to developers: - In squid.conf documentation I prepend each HTTP format code with [http::] Is it OK? Most of them yes. There are a few where http:: does not make sense at all: tsSeconds since epoch tusubsecond time (milliseconds) tlLocal time. Optional strftime format argument tgGMT time. Optional strftime format argument Definitely. dtTotal time spent making DNS lookups (milliseconds) uiUser name from ident usUser name from SSL Yeah, at least for now. These are also borderline: aClient source IP address AClient FQDN pClient source port laLocal IP address (http_port) lpLocal port number (http_port) AServer IP address or peer name Agreed. I'm fine with ignoring http:: for now if people do enter it for these, so no coding change. But I don't think we should document them that way. Agreed. I'd shift the clearly HTTP ones out to a section like you have ICAP. Leaving the rest prefix-ambiguous until something is done properly. Or at least remove the http:: prefix in the documentation from those mentioned above. I remove the http:: prefix for the format codes mentioned above. But maybe the use of http:: prefix needs more discussion... Sure. adaptation::sum_trs / adaptation::tt These are very different! Tt is a single value while adapt::sum_trs is a list (of individual tr's). Adapt::sum_trs is also symmetric with adapt::all_trs (also a list, but possibly with more entries). Yes they are not ideal, but they are there and what people are used to for now. We can minimize confusion a bit. OK. done for total_time and size I think at the point you do: +case LFT_HTTP_SENT_STATUS_CODE_OLD_30: +debugs(46, 0, WARNING: the \Hs\ formating code is deprecated use the \Hs\ instead); +break; you could also set lt-type = LFT_HTTP_SENT_STATUS_CODE; to reduce the need for that OLD_30 type being used outside the parse. Particularly during the config dump later it is useful to display what its supposed to be. Sounds good. My compiler (because it is very clever @#%$##$%#!) does not allow me to remove the LFT_HTTP_SENT_STATUS_CODE_OLD_30 from switch statements. Is that because of a lack of default: ? Never mind then. Just add a note saying what compiler complains if its removed so I don't get just as smart and do it :) I spy a goto that can die... case CLF_NONE: goto last; @@ -1458,7 +1863,15 @@ last: (void)0; /* NULL statement for label */ +} == equates to return;. Please check for others in that same function. I prefer to not touch it at this time. We can change it in trunk with a separate patch. Okay. Fair enough. Amos -- Please be using Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16 Current Beta Squid 3.1.0.9
Re: disable-loadable-modules
Robert Collins wrote:
On Thu, 2009-07-09 at 20:22 +1200, Amos Jeffries wrote:
Robert Collins wrote:
On Thu, 2009-07-09 at 19:40 +1200, Amos Jeffries wrote:
...
checking whether to use loadable modules... yes, implicitly
Is the test for whether to use them broken?
The code is to always enables them by default unless forced not to.
Everything would work just fine if libtool would only add the
$(builddir) to the path its trying to locate locally built things on.
It will if its been told that its using local libraries. I suspect we're
misusing something in libtool/automake to generate these symptoms.
-Rob
I thought I had something adding ${srcdir} to the path we pass it.
But builddir does not seem to be available, and srcdir is the wrong
place to find the built convenience lib.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16
Current Beta Squid 3.1.0.9
Re: disable-loadable-modules
On Fri, 2009-07-10 at 23:33 +1200, Amos Jeffries wrote:
I thought I had something adding ${srcdir} to the path we pass it.
But builddir does not seem to be available, and srcdir is the wrong
place to find the built convenience lib.
Neither should be done. The lib should be added (the .la file), and the
wrapper script for runtime execution sets LD_LIBRARY_PATH for us.
-Rob
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
On 07/10/2009 12:26 AM, Amos Jeffries wrote: adaptation::sum_trs / adaptation::tt These are very different! Tt is a single value while adapt::sum_trs is a list (of individual tr's). Adapt::sum_trs is also symmetric with adapt::all_trs (also a list, but possibly with more entries). You sure? I better be! I spent a while trying to figure out what all_trs and sum_trs were and it looked like all_trs was the list and sum_trs was the single-value sum of them. Here is the relevant documentation which, I believe, is accurate: tt Total server-side time in milliseconds. The timer starts with the first connect request (or write I/O) sent to the first selected peer. The timer stops with the last I/O with the last peer. adaptation_sum_trs Summed adaptation transaction response times recorded as a comma-separated list in the order of transaction start time. Each time value is recorded as an integer number, representing response time of one or more adaptation (ICAP or eCAP) transaction in milliseconds. When a failed transaction is being retried or repeated, its time is not logged individually but added to the replacement (next) transaction. See also: adaptation_all_trs. adaptation_all_trs All adaptation transaction response times. Same as adaptation_strs but response times of individual transactions are never added together. Instead, all transaction response times are recorded individually. Suggestions on how to document these better are welcomed, of course. If I did get it wrong, then the tt reduction still applies, but to the other option... Which one? Both adaptation_sum_trs and adaptation_all_trs are *lists* and not single values like tt. If you mean icap_total_time, then we are in agreement that it should be spelled icap::tt. I think that is what Christos has done already. Thank you, Alex.
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
On 07/10/2009 01:57 AM, Amos Jeffries wrote: Tsantilas Christos wrote: Alex Rousskov wrote: On 07/09/2009 12:06 AM, Amos Jeffries wrote: ** Please make fqdncache_entry / ipcache_entry public classes instead of functional structs. Some gcc complain and doxygen won't document their functions properly. Sorry, I am not sure what you mean. What is a functional struct? Do you want us to replace struct fqdncache_entry with class fqdncache_entry and add public:? No objections, of course, just wanted to minimize merging headaches by keeping out-of-scope changes to the minimum... I did not touch it. The struct used in many places inside squid3 Please do change. Let's compromise and change it in a separate patch because the change you are requesting is unrelated to the patch scope. The rest of the code fails to prefix the names with struct the typedef made that possible. Using class and public: is the C++ upgrade equivalent to the typedef and allows functions/methods inside the struct where I suspect the typedef did not. I am not sure that is a valid statement. IIRC, struct and class are equal citizens in C++ as far as C++ rules are concerned. One is not an upgrade of the other. Typedefs for structs are C legacy that are not needed (and harmful!) in C++ code. IIRC, I have removed them in this patch to make new method definitions look normal. Cheers, Alex.
Re: [PATCH] Enhanced access logging, added ICAP logging, bug #2495 fix.
I am committing a version 3 of the patch. Amos Jeffries wrote: Tsantilas Christos wrote: Alex Rousskov wrote: On 07/09/2009 12:06 AM, Amos Jeffries wrote: Tsantilas Christos wrote: ** Please make fqdncache_entry / ipcache_entry public classes instead of functional structs. Some gcc complain and doxygen won't document their functions properly. Sorry, I am not sure what you mean. What is a functional struct? Do you want us to replace struct fqdncache_entry with class fqdncache_entry and add public:? No objections, of course, just wanted to minimize merging headaches by keeping out-of-scope changes to the minimum... I did not touch it. The struct used in many places inside squid3 Please do change. The rest of the code fails to prefix the names with struct the typedef made that possible. Using class and public: is the C++ upgrade equivalent to the typedef and allows functions/methods inside the struct where I suspect the typedef did not. If there are code references to struct fqdncache_entry or struct ipcache_entry, they are well within scope of the change removing relevant typedefs. FWIW: I find none in the existing code. Which makes the change safe. OK done. .. My compiler (because it is very clever @#%$##$%#!) does not allow me to remove the LFT_HTTP_SENT_STATUS_CODE_OLD_30 from switch statements. Is that because of a lack of default: ? Yes, but it is not good idea to add a default: statement here. Never mind then. Just add a note saying what compiler complains if its removed so I don't get just as smart and do it :) OK I add the comment. I spy a goto that can die... case CLF_NONE: goto last; @@ -1458,7 +1863,15 @@ last: (void)0; /* NULL statement for label */ +} == equates to return;. Please check for others in that same function. I prefer to not touch it at this time. We can change it in trunk with a separate patch. Okay. Fair enough. Amos 3p1-plus-mod-v3.patch.gz Description: Unix tar archive
[PATCH] Limit X-Forwarded-For growth
Hello,
Should the attached patch go in? It prevents assertions (or worse)
during forwarding loops or carefully crafted messages. Production-tested
in Squid 3.0, although I do not know whether the code was ever triggered
outside the lab.
This change also prevents most cases of pointless computation of the
original X-Forwarded-For value list in Squid 3.1. That computation can
be quite expensive.
Thank you,
Alex.
Limit X-Forwarded-For growth.
X-Forwarded-For growth leads to String size limit assertions and probably
other problems.
To make growth-associated problems visible during forwarding loops, the
loop breaking code must be disabled (no Via) or not applicable (direct
forwarding) and request_header_max_size has to be raised or disabled.
The X-Forwarded-For header may also grow too large for reasons unrelated
to forwarding loops.
Ported from 3p0-plus (r8940..8941).
This change also prevents most cases of pointless computation of the original
X-Forwarded-For value list. That computation can be quite expensive.
=== modified file 'src/http.cc'
--- src/http.cc 2009-07-11 00:16:42 +
+++ src/http.cc 2009-07-11 01:07:15 +
@@ -1464,7 +1464,6 @@
LOCAL_ARRAY(char, ntoabuf, MAX_IPSTRLEN);
const HttpHeader *hdr_in = orig_request-header;
const HttpHeaderEntry *e = NULL;
-String strFwd;
HttpHeaderPos pos = HttpHeaderInitPos;
assert (hdr_out-owner == hoRequest);
@@ -1514,24 +1513,37 @@
}
#endif
-strFwd = hdr_in-getList(HDR_X_FORWARDED_FOR);
-
/** \pre Handle X-Forwarded-For */
if (strcmp(opt_forwarded_for, delete) != 0) {
+
+String strFwd = hdr_in-getList(HDR_X_FORWARDED_FOR);
+
+if (strFwd.size() 65536/2) {
+// There is probably a forwarding loop with Via detection disabled.
+// If we do nothing, String will assert on overflow soon.
+
+// XXX: the Right Thing is to catch this at a higher level and
+// terminate this looping transaction or it may loop forever.
+strFwd = unknowns;
+
+static int warnedCount = 0;
+if (warnedCount++ 100) {
+const char *url = entry ? entry-url() : urlCanonical(orig_request);
+debugs(11, 1, Warning: Undetected forwarding loop for url);
+}
+}
+
if (strcmp(opt_forwarded_for, on) == 0) {
/** If set to ON - append client IP or 'unknown'. */
-strFwd = hdr_in-getList(HDR_X_FORWARDED_FOR);
if ( orig_request-client_addr.IsNoAddr() )
strListAdd(strFwd, unknown, ',');
else
strListAdd(strFwd, orig_request-client_addr.NtoA(ntoabuf, MAX_IPSTRLEN), ',');
} else if (strcmp(opt_forwarded_for, off) == 0) {
/** If set to OFF - append 'unknown'. */
-strFwd = hdr_in-getList(HDR_X_FORWARDED_FOR);
strListAdd(strFwd, unknown, ',');
} else if (strcmp(opt_forwarded_for, transparent) == 0) {
/** If set to TRANSPARENT - pass through unchanged. */
-strFwd = hdr_in-getList(HDR_X_FORWARDED_FOR);
} else if (strcmp(opt_forwarded_for, truncate) == 0) {
/** If set to TRUNCATE - drop existing list and replace with client IP or 'unknown'. */
if ( orig_request-client_addr.IsNoAddr() )
@@ -1543,7 +1555,6 @@
hdr_out-putStr(HDR_X_FORWARDED_FOR, strFwd.termedBuf());
}
/** If set to DELETE - do not copy through. */
-strFwd.clean();
/* append Host if not there already */
if (!hdr_out-has(HDR_HOST)) {
Re: [PATCH] Limit X-Forwarded-For growth
Alex Rousskov wrote: Hello, Should the attached patch go in? It prevents assertions (or worse) during forwarding loops or carefully crafted messages. Production-tested in Squid 3.0, although I do not know whether the code was ever triggered outside the lab. This change also prevents most cases of pointless computation of the original X-Forwarded-For value list in Squid 3.1. That computation can be quite expensive. Thank you, Alex. I think it should. Just one thing: On the error case adds: strFwd = unknowns; IIRC 'unknown' is specified, but maybe wrong. If any text is able to be added there loop error may be better to set to show whats gone wrong. If this is just a typo of 'unknown' please correct on commit. bb:approve Amos -- Please be using Current Stable Squid 2.7.STABLE6 or 3.0.STABLE16 Current Beta Squid 3.1.0.9
