Squid crashes on shutdown while cleaning up idle ICAP connections.
The global Adaptation::Icap::TheConfig object is automatically
destroyed when Squid exits. Its destructor destroys Icap::ServiceRep
objects that, in turn, close all open connections in the idle
connections pool. Since this happens after comm_exit has destroyed all
Comm structures associated with those connections, Squid crases.
Amos Jeffries [Tue, 1 Mar 2016 02:57:50 +0000 (15:57 +1300)]
RFC 7725: Add registry entry for 451 status text
While Squid does not generate these messages automatically we still have
to relay the status line text accurately, and admin may want to use it
for deny_info status.
After certain failures, FwdState::retryOrBail() may be called twice,
once from FwdState::unregisterdServerEnd() [called from
HttpStateData::swanSong()] and once from the FwdState's own connection
close handler. This may result in two concurrent connections to the
remote server, followed by an assertion upon a connection closure.
This patch:
- After HttpStateData failures, instead of closing the squid-to-peer
connection directly (and, hence, triggering closure handlers), calls
HttpStateData::closeServer() and mustStop() for a cleaner exit with
fewer wasteful side effects and better debugging.
- Creates and remembers a FwdState close handler AsyncCall so that
comm_remove_close_handler() can cancel an already scheduled callback.
The conversion to the AsyncCall was necessary because legacy [close
handler callbacks] cannot be canceled once scheduled.
Marcos Mello [Tue, 23 Feb 2016 22:27:43 +0000 (11:27 +1300)]
Bug 3826: SMP compatibility with systemd
** These changes require capabilities changes specific to Squid-4 and
require systemd 209+
NOTE: 'squid -z' command does not yet support SMP with systemd.
Differences from the Squid-3 tools/systemd/squid.service:
- After=nss-lookup.target, for people running a local DNS server like BIND.
Since there is no requirement dependency, it is a NOP when no such
service is running.
- Type=forking and squid without -N in ExecStart: SMP now works.
- PIDFile=/var/run/squid.pid to tell systemd what pid is the main one. This
is actually optional with Squid 4, because systemd will consider its first
child as the main pid. But let's be safe. DEFAULT_PID_FILE could be used
here with proper autoconf/automake magic...
- ExecReload calls kill rather than 'squid -k reconfigure'. systemd already
knows the main pid.
- KillMode=mixed. The old KillMode=process sends SIGTERM (and SIGKILL after
TimeoutStopSec) only to main daemon process. 'mixed' OTOH sends SIGTERM
only to main process, but SIGKILL to all services' cgroup processes after
timeout. With 'mixed' systemd ensures if daemon shutdown fails it will
clean up all the remains. 'mixed' requires systemd >= 209.
Alex Rousskov [Fri, 19 Feb 2016 21:26:00 +0000 (14:26 -0700)]
Fix propagation of response status line parsing error details.
This is a follow-up patch to trunk r14548 (Bug 4432). Now that the
calling code is using the right field to get the parsing error details
(parseStatusCode), we need to fix the code that sets those parsing error
details [in case of response status line parsing errors].
TODO: To minimize chances of similar "I forgot to set parseStatusCode"
bugs slipping through, hide that data member behind a method that
returns scInvalidHeader (or a new scInternalSquidError) if parseError_
is still zero. Rename parseStatusCode to parseError_ and stop confusing
it with the response status code.
Alex Rousskov [Fri, 19 Feb 2016 21:23:08 +0000 (14:23 -0700)]
Throw instead of asserting on some String overflows.
Note that Client-caught exceptions result in HTTP 500 (Internal Server
Error) responses with X-Squid-Error set to "ERR_CANNOT_FORWARD 0".
Also avoid stuck Client jobs on exceptions. See trunk r8266 for a
similar fix with a detailed discussion. Here, I added doneWithFwd
instead of setting fwd to NULL because we dereference fwd (and store
pointers to things stored in fwd!) in many places. I think it is too
risky to just clear refcounted FwdState pointer (except in the
destructor where doing so is pointless).
Using doneWithFwd correctly is difficult because there are many ways we
can be "done" with FwdState, including:
* calling fwd->complete(),
* calling fwd->handleUnregisteredServerEnd(), and
* closing the connection that FwdState monitors for closures.
The latter is especially tricky case because the closing is initiated in
many places, the process is asynchronous, and not all control
connections are monitored by FwdState.
For example, the updated control connection closure handler assumes that
it is being used for either external closures or internal closures
incorrectly used instead of mustStop()/abortAll(). In both cases, either
FwdState is still monitoring the connection (OK) or we forgot to call
one of its "done" methods listed above before closing. The latter would
be a bug, but I did not find any signs of it and fixing it would be
outside this change scope anyway.
Also unified String size limit checks [that I could find].
external_acl parameters separated by %20 instead of space
If an external ACL is configured with more than one parameter as shown
in the example below, then Squid sends those parameters to the
external_acl helper separated by %20 characters instead of spaces:
acl TEST external ACLTYPE param1=val1 param2=val2
This change fixes regression introduced in trunk r14351 (Support
logformat %macros in external_acl_type format) but more work may
be needed to make Squid behave as squid.conf.documented promises.
Amos Jeffries [Fri, 19 Feb 2016 15:06:42 +0000 (04:06 +1300)]
Revert r14303: Migrate StoreEntry to using MEMPROXY_CLASS
This change has been identified as the trigger for several object caching
errors. The real cause is not yet known, but reverting this optimisation
avoids it, so is being done for stability.
This resolves bugs 4370 and maybe also 4354 and 4355
William Lima [Thu, 18 Feb 2016 12:48:08 +0000 (01:48 +1300)]
Bug 3870: assertion failed: String.cc: 'len_ + len <65536' in ESI::CustomParser
The custom ESI parser used in absence of libxml2 or libexpat parsers was
restricted to handling 64KB buffers but under some conditions could expand
to over 64KB during the parse process. Hitting this assertion.
TODO: the parser can now be redesigned to make use of Tokenizer and
CharacterSet parsing tools. But that is left for later work.
* Do not use parsing leftovers, such as HTTP response status code. Doing
so screws up error detection logic in continueAfterParsingHeader() and
leads to stuck transactions instead of error responses.
* Do not store the fake half-baked response (via replaceHttpReply).
Doing so leads to assertions. The fake response is only meant for
continueAfterParsingHeader().
I also removed a misleading XXX about connection closure. Our
continueAfterParsingHeader() handles errors, not processReplyHeader().
TODO: The error detection/propagation code is ugly and should be
rewritten [using C++ exceptions].
tangqinghao [Thu, 18 Feb 2016 02:48:41 +0000 (15:48 +1300)]
Bug 4111: leave_suid() does not properly handle error codes returned by setuid
... this will cause privilege escalation in the rare case that setuid fails.
So far there are no known cases of this happening when downgrading from root.
Also fixes several incorrect uses of errno which may have been obscuring
error message details if it did happen.
Amos Jeffries [Wed, 17 Feb 2016 21:03:29 +0000 (10:03 +1300)]
SourceLayout: Move the Runner which manages SSL SessionCache to libsecurity
Unfortunately the OpenSSL session cache callbacks cannot also be moved
due to circular dependency issues. However, when those are resolved by
later libsecurity API additions the callbacks will be much easier to
shift. For now the three symbols shared between the two libraries are
exposed by libsslsquid in the Ssl:: namespace.
Cache initialization is now moved into the Runner. Binding its state
initialization more tightly to the memory allocation and initialization.
Which also removes the need for explicit main.cc dependency.
One issue was uncovered during this:
* While ssl/support.h was defining a destruct_session_cache() function
that appeared to release the cache memory, it was not actually being
used anywhere. Which unless a fortuitous sequence of events is happening
means that the memory for the cache entries may not be released properly.
On the other hand the cache should only be erased on shutdown so the
effects of this are minor.
The unused function has been removed and the issue is now expicitly
noted in the Runner shutdown handling method for future investigation.
In the case SSL errors detected by certificate validator helper the objects
stored in Ssl::ServerBump::sslErrors member and will never released.
This member normally points to an Ssl::CertErrors list attached to the related
SSL object which is responsible to release this list.
When the cert validator detects errors a new errors list created and attached
to the related Ssl::ServerBump::sslErrors member but the SSL objects still
hold the old one. The old list released but not the new one.
This patch also fixes the case the cbdata protected Ssl::CertErrors list,
still is used through the related Ssl::ServerBump object but it is not valid
any more, because the SSL object which hold it gone.
This patch instead of storing the Ssl::CertErrors list to Ssl::ServerBump
object stores the SSL object and increases its reference to avoid be released
Bug 4437: Fix Segfault on Certain SSL Handshake Errors
Squid after an unsuccesfull try to connect to the remote server may make two
concurrent retries to connect to the remote SSL server, calling twice the
FwdState::retryOrBail() method, which may result to unexpected behaviour.
Prevent this by just closing the connection to the remote SSL server inside
FwdState::connectedToPeer method on error and instead of calling the
FwdState::retryOrBail method, just allow comm_close handler to retry the
connection if required.
This bug investigated after the r14528 patch.
The (forgotten?) Ssl::PeekingPeerConnector::callback member is always NULL and
hides the callback member of the parent Ssl::PeerConnector class. This is has
as result the failure of "Must(callback != NULL)" clause inside the
Ssl::PeekingPeerConnector::tunnelInsteadOfNegotiating method investigated with
the r14528 patch.
Amos Jeffries [Wed, 10 Feb 2016 09:36:47 +0000 (22:36 +1300)]
Cleanup: const correctness for SBuf iterators
The SBuf iterator has almost all properties of a const_iterator but the
begin/end methods producing it had the syntax for returning a non-const
iterator. This leads to several const related problems despite the
SBufIterator providing a const API:
1) range-for loops cannot be used on const SBuf
2) begin/end/rbegin/rend operators cannot be used on const SBuf
Also, the SBufIterator API for operator*() used a temporary char
which, wile const, allows the compiler to implicitly use move semantics
on the return value and allows some (thankfully unused) &*itr() syntax
to compile when it should not be possible.
To avoid even the potential of that causing hidden issues in future we
convert the operator*() to the API definition used widely within the STL
const_iterator's (const char &operator*() const).
Amos Jeffries [Tue, 9 Feb 2016 08:57:33 +0000 (21:57 +1300)]
Cleanup: convenience library renaming
I have been trying to automate graphing of the Squid internal
dependencies. One of the major issues that has encountered is that some
of our convenience libraries use the '-' hyphen character which is a
reserved character in DOT graph format.
To make the scripts much simpler and the visual output reflect exactly
what the library names are this patch cleans up the libraries to follow
our pre-existing policy, and now also to remove punctuation from library
names. Which condition has been added to the guidelines documentation.
The information about PeekingPeerConnector splicing the connections
was lost in some cases, resulting in two different bugs:
- With a certificate validator, the PeekingPeerConnector class calls
back FwdState, which calls the ConnStateData class, which then tries
secure the connection with the already tunneled SSL client and
closes the connection on negotiating errors.
- Without a certificate validator, the PeekingPeerConnector class
never calls FwdState class, and both PeekingPeerConnector and
FwdState objects stall until finishing tunnelState closes server
and client connections.
Now, PeerConnector always calls FwdState back, marking spliced
connections as such. This has the following positive side-effects:
- When FwdState learns about spliced connections, it does not call
ConnStateData back. Instead, it terminates and gets destroyed.
The tunnel continues uninterrupted.
- The PeekingPeerConnector job ends and is destroyed instead of
waiting to call FwdState.
Fix external_acl problems after trunk r14351
(Support logformat %macros in external_acl_type format).
The above changes created the following problems:
- external_acl requires AccessLogEntry but ALE is not available
in many cases such as ssl_bump ACLs.
- The %<cert_subject stopped working because it was supported by
external_acl code and not by logformat code.
This patch:
- Passes AccessLogEntry in most cases.
For example, PeerConnector-related classes are now covered.
- Implements the %<cert_subject formating code for logformat.
Amos Jeffries [Sun, 31 Jan 2016 12:05:30 +0000 (01:05 +1300)]
SourceLayout: rename ClientSocketContext to Http::Stream
... and provided through http/libsquid-http.la.
The name is chosen to match the RFC7540 HTTP/2 "stream" terminology.
Which defines a stream as a bi-directional transaction, including request,
reply and all related 1xx informational and/or control messages.
That same word "stream" is also used in RFC7230 briefly to describe the
same "transaction" scope and details. But not formalized until RFC7540.
Http::Stream's may be initiated by a client HTTP request, Squid internally,
or in HTTP/2 a server PUSH_PROMISE frame.
There are no logic changes in this. Just symbol renaming and move.
projects / thirdparty / squid.git / log
