On 22/01/2013 9:15 a.m., Tsantilas Christos wrote:
On 01/18/2013 08:37 PM, Alex Rousskov wrote:
On 01/18/2013 08:14 AM, Steve Hill wrote:
The attached patch enables autoconsumption of the BodyPipe if access
isn't allowed in order to discard the request body rather than letting
it fill the buffers.
Please note that the patch enables auto-consumption for all callout
errors, not just access denials.
Also, the trunk code is even more complex in this area. I believe your
patch, when carefully ported to trunk, may enable auto-consumption for
all callout errors *that have readNextRequest set*. That makes sense to
me: if we are going to read the next request, we should consume the
current one without closing the client connection so we should not call
expectNoForwarding() that may have that side effect.
The overall intent sounds correct to me. Even if we do not want to keep
the connection open after responding with an error, it may be impossible
to deliver the error message to a half-broken client that does not read
while writing to us.
However, this brings us back to trunk r11920 and bug 3420. While that
fix was triggered by an assertion, it is clear from the code comments
and the commit message that we were worried about a client tying Squid
client connection "forever" (with or without sending data). Your changes
will re-enable that behavior in some cases, I guess. More on that below.
I'm not entirely sure this is the best fix - calling
expectNoForwarding() would seem to be neater, but this also causes the
client-side socket to close, which would break authentication mechanisms
that require the connection to be kept alive. Hopefully someone with a
better understanding of the Squid internals can review the patch and
tell me if it is the right approach?
Thank you very much for treading carefully and disclosing potential
problems!
Indeed, having two methods with approximately the same purpose and a
hidden critical distinction is asking for trouble. A lot of existing
expectNoForwarding-path code comments describe the situation that
matches yours. Moreover, as far as I can see, we may call the old
expectNoForwarding() first and then call your forwardToBitbucket()
later, all for the same error!
As far as I can tell, we have several cases to take care of (or ignore)
here:
1. Authentication: Must read next request on the same connection so must
have readNextRequest set (if not, this is probably a bug). Have to live
with the possibility of the client sending forever.
In this case we may have problem only for NTLM or similar authentication.
But in this case imagine that the HTTP client post a huge file and Squid
respond with an "401 Unauthorised". Is it normal to read all of the
file in this case?
"Maybe.". Some clients work okay some don't. Like IE6 requires it and
Sharepoint, Exchange, etc do not work if its sent.
We are going to read all of the file in the next
authenticated request too. So for one request we have to read ALL the
posted file twice. It is not normal.
I believe that the best is to send the error page and abort. The browser
should handle early replies to support such cases...
Also the curl version I have supports early replies, I used custom perl
client to reproduce the problem...
HTTP allows us to close a connection at any time. For NTLM handshakes we
can (configurably) close the connection after the first 401 message, but
not the second, and are again on the third request - which should have
succeeded.
Amos
