Author: Alex Rousskov <rousskov@measurement-factory.com>
Bug 3420: Request body consumption races and !theConsumer exception.
Also fixes endless waiting for HTTP client to send req body we no longer need.
Before these changes, the client side used a single "closing" state to
handle two different error conditions:
1. We stopped receiving request body because of some error.
2. We stopped sending response because of some error.
When a "directional" error occurred, we try to keep the transaction going in
the other direction (e.g., to give ICAP the entire request or to give HTTP
client the entire response). However, because there was just one "closing"
state, the code failed to correctly detect or process many corner cases,
resulting in stuck transactions and !theConsumer assertions/exceptions due to
races between enableAutoConsumption() and expectNoConsumption() calls.
This patch replaces the "closing" state with two direction-specific "we
stopped sending/receiving" flags.
Now, when the response sending code is done, it now checks whether the
receiving code stopped and closes the connection as needed. This is done both
when we encounter a sending error (ClientSocketContext::initiateClose) and
when we successfully sent the entire response to the client
(ClientSocketContext::keepaliveNextRequest).
Similarly, when the request body reading code is done, it now checks whether
the receiving code stopped and closes the connection as needed. This is done
both when we encounter a receiving error
(ConnStateData::noteBodyConsumerAborted) and when we successfully receive the
entire request body from the client (ClientSocketContext::writeComplete).
TODO: This patch focuses on various error cases. We might still have problems
when there is an early HTTP response and no errors of any kind. I marked the
corresponding old code with an XXX.
External ACL sometimes cannot find the credentials in ACL Checklist even
if they are attached to the HTTPRequest object.
This seems to happen when the checklist is created and the line match
started before the credentials are known. The credentials validation
updates the HTTP request state but is not aware of ACL checklists needing
to be updated so it never happens.
This patch:
* locate the %LOGIN value from either place where credentials can be found,
* updates the checklist if it was unset,
* passes '-' to the helper if no credentials at all were given.
Although the earlier logics forcing a lookup means this '-' case should
not happen it might if the external ACL were processed in 'fast' check.
Amos Jeffries [Sun, 4 Dec 2011 05:43:42 +0000 (22:43 -0700)]
Add FdeCbParams parameter object to CommCalls API.
The problem:
CommCalls API functionality is conflated with comm operational calls
created to do general FD handling (FD as pipe handle, FD as disk handle,
FD as pointer into the fd_table structure). Sometimes because they do
operations mirroring comm handlers and also use FD. None of this actually
requires the CommCalls layer to be involved though. The Comm::Connection
objects which CommCall TCP handlers pass around is also very inappropriate
for these FD types.
This adds FdeCbParams to CommCalls infrastructure, for use internally and
"lower" than comm API to pass around raw FD values. This should be avoided
on TCP socket FD, but may be used by callers needing FD where
Comm::Connection is inappropriate.
Amos Jeffries [Sun, 4 Dec 2011 05:39:39 +0000 (22:39 -0700)]
CBDATA call Dialer template
This adds a template for dialing Unary CBDATA wrapper functions with
type safety. Avoiding the casting that currently occurs in wrappers and
allowing the AsyncCall APIs to be used for these callbacks.
Bug fix: HttpRequest::flags.intercepted, HttpRequest::flags.spoof_client_ip neve
r set
The request_flags::intercepted,request_flags::spoof_client_ip are 1 bit integers
so when you are try to set to an integer bigger than 1 will overflow and the
results will not be what you are expecting.
Amos Jeffries [Tue, 22 Nov 2011 12:00:59 +0000 (01:00 +1300)]
Cleanup: comm Close handlers
Make handlers take the CommCloseCbParams instead of series of expanded
variables.
Opening access to the other CommCommonCbParams fields with Connection/FD
data. Hiding the deprecated FD parameter from most handlers. Which seem
not to have actually needed it in most cases outside Comm.
The StoreEntry::write in the case of an empty write, calls the StoreEntry
handlers. It is possible one of these handlers will change the state of the
store entry or abort it. The next call of the StoreEntry::write will cause
this assertion.
The block of code which calls the StoreSntry handlers in the case of an empty
write, added to allow forward http headers to the client even if no body data
arrived yet (bug 1750). There is not need for this part of code in the latest
squid releases, so it is safe to be removed.
Alex Rousskov [Mon, 21 Nov 2011 16:49:34 +0000 (09:49 -0700)]
Avoid crashes when processing bad X509 common names (CN).
X509_REQ_get_pubkey() returns a refcounted object that we must clean after use.
X509_REQ_get_subject_name() does not; cleaning the result may cause segfaults.
How we are supposed to tell the difference is beyond me.
author: Martin Huter <mhuter@barracuda.com>, Alex Rousskov <rousskov@measurement-factory.com>, Christos Tsantilas <chtsanti@users.sourceforge.net>
Bug 2619: Excessive RAM growth due to unlimited adapted body data consumption
If the client does not read from the open connection (i.e. the user does not
confirm the browsers download-message-box in microsofts IE), squid keeps on
reading data from the ICAP server into the store entry, while no more data
can be delivered to the client.
Thus the store entry in memory is growing and squid may - in worst case -
consume memory up to the size of the users download.
This patch add API to StoreEntry to call the producer back when released
memory/space from the StoreEntry and add code to the ICAP client code to not
consume body data comes from the ICAP server when there is not available space
in the store entry.
Amos Jeffries [Fri, 18 Nov 2011 07:48:25 +0000 (00:48 -0700)]
Log Format token namespace upgrade
This updates the format parser and storage objects in the Format::
namespace and separates some into separate files.
Add a registration API so components can register themselves an array
of tokens in a namespace. Registering the arbitrary namespace "example"
with some tokens ("a","b") will cause the parser to accept those tokens
in a logging format like so: "%example::a %example::b".
Future work:
- use runners registry instead of Init() function
- convert the error pages to use format for the page body macros
- convert the %ssl_* tokens in src/ssl/* to use format and "ssl::"
- convert external_acl_type to use formats for its helper input string.
Mathias Fischer [Thu, 17 Nov 2011 15:31:57 +0000 (08:31 -0700)]
Use the right certificate when detailing SSL certificate validation errors.
When an _intermediate_ SSL server certificate fails validation, we should
report errors using information in that certificate and not in the top-level
"peer" certificate. Otherwise, our details may make no sense. For example, we
may say that the validation failed due to the expired certificate and then show
an expiration date in the future (because the top-level certificate did not
expire but the intermediate certificate did).
OpenSSL X509_STORE_CTX_get_current_cert() returns the certificate that was
being tested when our certificate validation callback was called.
Alex Rousskov [Tue, 15 Nov 2011 18:21:07 +0000 (11:21 -0700)]
Fix Comm::Write closing() assertion when retrying a failed UDP DNS query.
When we receive a UDP DNS response with a truncation (TC) bit set, we retry
using TCP. Since the retry trigger has nothing to do with the TCP connection,
it is possible that the TCP connection is being closed when we are about to
write to it: A call to our connection close callback has been scheduled but
has not fired yet. We must check for and avoid such race conditions.
projects / thirdparty / squid.git / log
