On 01/21/2013 07:04 AM, Steve Hill wrote: > I have changed to using expectNoForwarding() and removed the > stopReceiving() call from noteBodyConsumerAborted() and this appears to > work correctly:
Good, thank you for checking this! We just need to decide whether reading the entire request is what we want in all cases. The earlier changes were done to stop Squid reading that request (because reading the whole requests wastes a lot of resources if the request is huge or, worse, malicious). > I don't really understand the purpose of the readMore flag, can you > explain it? It is one of the factors that Squid connection context uses to read the new request from the client connection. ConnStateData::clientParseRequests() is key here. Please note that Squid connection context is not the only object that may do reading and that, in theory, that context does not deal with reading request bodies. See also: mayUseConnection(). The whole thing is complex, messy, and, hence, buggy. >> * Discuss and possibly remove readNextRequest setting from >> ClientHttpRequest::handleAdaptationFailure(), to remove inconsistency >> discussed in #2 above. > > As mentioned above, I don't understand what readMore is actually for. It is difficult (for me) to describe all its current [side] effects, which is a part of the problem, but it is close to "Squid client side needs to read more incoming data, either for the current request or to get the next request". > However, as I mentioned in my previous email, the only time I can see us > wanting to blindly close the connection for adaptation failures is when > we've already started sending a response to the client and the > adaptation service blows up in the middle of the response body. It > doesn't _look_ like handleAdaptationFailure() handles that event though, > unless I'm mistaken? Yes, handleAdaptationFailure() may be called when adaptation fails in the middle of the response. However, handleAdaptationFailure() is for REQMOD adaptations (which may overlap with RESPMOD adaptations in some cases!). >> * Discuss whether we are OK with clients tying up Squid after an >> [authentication] error. Perhaps we need another timeout there, perhaps >> there are already mechanisms to address that, or perhaps we do not >> really care. > 4. The client makes a request that generates an error, uses chunked > encoding and sends an infinitely long body. > I haven't tested (4), but I have no reason to believe there's anything > that currently stops it from happening. I'm not sure whether or not we > should do something to prevent it - that's one for discussion. Yes, (4) is the interesting case here. We do prevent (4) on certain adaptation errors. This is what prompted the original changes in this area! It feels wrong to just undo those changes without a discussion (because they were deemed useful because they were solving a real problem for some users at the time). However, I agree that we do not prevent (4) in many (all?) other cases. So, should we a) undo the existing limited protection (because it is limited and conflicts with the cases were we do want to read the whole thing, at least as far as code is concerned); b) expand the existing protection to all cases (because it is better than allowing case #4); c) adjust behavior based on existing and/or new configuration options, to make the choice between a and b depend on the current circumstances and expand the configurable protection to all cases; or d) polish and apply your changes, resulting in protection for the previously covered cas(es) and no added protection for other cases. Wait for somebody to care enough about a more comprehensive and consistent solution to make it happen. This is essentially what my suggestions were meant to accomplish if "discuss" items lead nowhere. > On 01/21/2013 04:19 PM, Amos Jeffries wrote: >> If the data is being bropped on arrival by Squid we should be watching >> for these cases and aborting client connections which go on too long. I >> suspect the read_timout timer should be kept ticking on these reads and >> not reset to 0. That would cap the abuse time at 15 minutes per client >> rather than infinite. This idea is for option (c) above. I like it a lot because the proposed behavior is meaningful, flexible, and does not require new options. Steve, do you think you have the cycles to implement Amos' suggestion? It is not going to be easy, but it would be the best outcome of this discussion (that I can think of). Thank you, Alex.
