Squid crashes on shutdown while cleaning up idle ICAP connections, part2
Fixes to allow make check work:
- Fix tests/stub_pconn.cc to correctly implement a stub IdleConnList
- Replace reinterpret_cast with static_cast to avoid errors when clang is used.
The IdleConnList is now a hash_link kid and static_cast is the correct one.
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 crashes.
This patch uses updated RegisteredRunners API to close all connections
in existing connections pools before Squid cleans up the Comm subsystem.
Also added a new IndependentRunner class to the RegistersRunner API,
which must be used for runners that are destroyed by external forces,
possibly while still being registered. IndependentRunner objects
unregister themselves from Runners registry when they are destroyed.
The finishShutdown() method is now virtual and may be overloaded to
implement independent runner cleanup during main loop (and/or to support
complex cleanup actions that require calling other virtual methods). The
method is still called for all registered runners but it no longer
destroys the runner. For dependent runners, the destruction happens soon
after the finishShutdown event ends but also during the main loop
(unless the runner has managed to register after the main loop ended).
HTTP: do not allow Proxy-Connection to override Connection header
Proxy-Connection header is never actually valid, it is relevant in
HTTP/1.0 messages only when Connection header is missing and not
relevant at all in HTTP/1.1 messages.
This fixes part of the behaviour, making Squid use Connection header
for persistence (keep-alive vs close) checking if one is present
instead of letting Proxy-Connection override it.
TODO: Proxy-Connection still needs to be ignored completely when
the message version is HTTP/1.1.
Wrong error_depth value printed with %ssl::<cert_errors
This patch fixes the following problems:
- Squid stores the depth information only for the first certificate error
- The Ssl::CertError assignment operations does not update depth member
- The Ssl::CertError equal/not equal operators does not check depth information
SSL CN wildcard must only match a single domain component [fragment].
When comparing the requested domain name with a certificate Common Name,
Squid expanded wildcard to cover more than one domain name label (a.k.a
component), violating RFC 2818 requirement[1]. For example, Squid
thought that wrong.host.example.com matched a *.example.com CN.
[1] "the wildcard character * ... is considered to match any single
domain name component or component fragment. E.g., *.a.com matches
foo.a.com but not bar.foo.a.com".
In other contexts (e.g., ACLs), wildcards expand to all components.
matchDomainName() now accepts a mdnRejectSubsubDomains flag that selects
the right behavior for CN match validation.
The old boolean honorWildcards parameter replaced with a flag, for clarity
and consistency sake.
This patch also handles the cases where the host name consists only from dots
(eg malformed Host header or SNI info). The old code has undefined behaniour
in these cases. Moreover it handles the cases a certificate contain zero length
string as CN or alternate name.
HTTP: MUST always revalidate Cache-Control:no-cache responses.
Squid MUST NOT use a CC:no-cache response for satisfying subsequent
requests without successful validation with the origin server. Squid
violated this MUST because Squid mapped both "no-cache" and
"must-revalidate"/etc. directives to the same ENTRY_REVALIDATE flag
which was interpreted as "revalidate when the cached response becomes
stale" rather than "revalidate on every request".
This patch splits ENTRY_REVALIDATE into:
- ENTRY_REVALIDATE_ALWAYS, for entries with CC:no-cache and
- ENTRY_REVALIDATE_STALE, for entries with other related CC directives
like CC:must-revalidate and CC:proxy-revalidate.
Reusing ENTRY_CACHABLE_RESERVED_FOR_FUTURE_USE value for the more
forgiving ENTRY_REVALIDATE_STALE (instead of the more aggressive
ENTRY_REVALIDATE_ALWAYS) fixes the bug for the already cached entries -
they will be revalidated and stored with the correct flag on the next
request.
Alex Rousskov [Tue, 6 Sep 2016 02:45:20 +0000 (14:45 +1200)]
HTTP: validate Content-Length header values
Squid is violating HTTP MUSTs by forwarding messages with
problematic Content-Length values. Some of those bugs were fixed in
trunk r14215. This change handles multiple Content-Length values inside
one header field, negative values, and trailing garbage. Handling the
former required a change in the overall Content-Length interpretation
approach (which is why it was previously left as a TODO).
Squid now passes almost all Co-Advisor tests devoted to this area. We
are not 100% done though: We still need to handle malformed values with
leading signs (e.g., "-0" or "+1"). However, I hope that the remaining
problems are relatively minor. I do not plan on addressing them in the
foreseeable future.
Also improved httpHeaderParseOffset(): Added detection of overflowing
and underflowing integer values; polished malformed value detection code
(Linux strtoll(3) manual page has a good example). The function no
longer considers empty strings valid and reports trailing characters.
The function still accepts leading whitespace and signs. It is still the
wrong approach to HTTP numbers parsing, but further improvements are out
of scope because they are complicated and would require significant
caller rewrites.
MUST respond with 414 (URI Too Long) when request target exceeds limits.
Before the fix, Squid simply closed client connection after receiving a
huge URI (or a huge request-line), violating the RFC 7230 MUST. This
happened because a high-level Must(have buffer space) check in
ConnStateData::clientParseRequests() would throw an exception. Now these
problems are detected inside the low-level RequestParser code, where we
can distinguish huge URIs from huge methods.
Alex Rousskov [Fri, 19 Aug 2016 02:32:01 +0000 (20:32 -0600)]
Do not log error:transaction-end-before-headers after invalid requests.
Squid was not consuming read leftovers after failing to parse a request.
Starting with r14752, those leftovers were misinterpreted as another
unparsed request, creating an extra error:transaction-end-before-headers
access.log line after every error:invalid-request line (and probably
after every error:request-too-large line).
To stop Squid from accidentally reading new bytes and misinterpreting
them as another request, I was tempted to also clear flags.readMore
after consuming unparsable leftovers. In my tests, the flag is cleared
in ConnStateData::quitAfterError() called from clientTunnelOnError(),
but that logic looks rather fragile. I resisted the temptation to
improve it because controlling reads is a complicated matter (especially
in on_unsupported_protocol context) outside this logging fix scope.
Alex Rousskov [Wed, 17 Aug 2016 23:11:56 +0000 (17:11 -0600)]
Do not access-log chunked non-persistent responses with _ABORTED suffix.
The lack of Content-Length response header and STREAM_FAILED are not
closely related. ClientStreams should be capable of delivering complete
[chunked] responses without Content-Length header. If the peer response
was not chunked and lacked Content-Length, then we will return
STREAM_FAILED when false checkTransferDone() indicates that we did not
get everything yet but flags.complete forces us to stop sending.
Alex Rousskov [Wed, 17 Aug 2016 22:46:38 +0000 (16:46 -0600)]
Do not access-log SslBump-faked CONNECTs with _ABORTED suffixes.
All successful transactions, including fake CONNECTs, must be removed
from the pipeline (via a finished() call) to avoid _ABORTED suffix
added by Pipeline::terminateAll() which always calls noteIoError().
Also added a check that the remaining request in the pipeline is the
expected CONNECT. Old comments said as much but the code did not throw
if a different request was pipelined (and asserted if more than one
requests were). We now throw in all these problematic cases. I cannot
track all ConnStateData::getSslContextStart() callers deep enough to
be sure that only a single CONNECT can get through, but the change
reflects the expected behavior (and it may be useful to discover any
reality deviations from our expectations).
Amos Jeffries [Wed, 17 Aug 2016 00:38:25 +0000 (12:38 +1200)]
Better support for unknown URL schemes
Squid already contains AnyP::PROTO_UNKNOWN support for unknown protocols
but currently does not preserve the actual string value received for them.
This adds a textual representation ('image') to the UriScheme object to
fill that gap and ensure that all URL representations (ie cache keys,
logs and outgoing messages) are generated with the scheme string as it
was received rather than implicitly via a registered protocol type.
Future work:
* add ACL support for arbitrary scheme names
* support for comparisons of unknown schemes
Alex Rousskov [Sun, 14 Aug 2016 01:17:57 +0000 (19:17 -0600)]
Fixed build on systems needing explicit <utility> in RegexPattern.cc.
... after r14795.
Also removed bogus comments implying that std::move() does something to
its argument. That function is just a type cast; it does nothing else!
We use std::move() so that the compiler picks a move constructor or
assignment (where available). In case of RegexPattern::flags, using
std::move() has no effect because integers do not have those move
methods. However, it may be a good idea to use std::move() anyway
because the type of RegexPattern::flags might change in the future and
for consistency sake. Thus, I did not remove std::move(), only comments.
Amos Jeffries [Sat, 13 Aug 2016 10:12:06 +0000 (22:12 +1200)]
Bug 4561: Replace use of default move operators with explicit implementation
The '= default' syntax for move assignment operator and constructor can
result in double-free crashes when the class type containspointer members
since the trivial move uses std::memmove() which does not perform the
required zeroing/nullptr of the temporary source object being moved.
Make Squid death due to overloaded helpers optional.
Added on-persistent-overload=action option to helpers. Helper overload
is defined as running with an overflowing queue. Persistent helper
overload is [still] defined as being overloaded for more than 3 minutes.
The default behavior is unchanged(*) -- Squid worker dies with a fatal
error at the attempt to submit a new request to a persistenly overloaded
helper. This default behavior can also be configured explicitly using
on-persistent-overload=die.
With on-persistent-overload=ERR, when dealing with a persistently
overloaded helper, Squid immediately skips the helper request and sends
an ERR response to the caller. Squid informs the admin when it starts
and when it stops skipping helper requests due to persistent overload.
The code had conflicting notions of an "overloaded helper". The external
ACL helper, the URL rewriter, and the store ID code used queueFull() to
test whether the new request would overflow the queue (and, hence,
overload the helper), but queueFull() itself did not check whether the
queue was full! It checked whether the queue was already overflowing.
This confusion resulted in that code scheduling one extra helper request
before enabling bypass. The code and its documentation are now more
consistent (and better match the "overload" terminology used by the new
configuration option, which also feels better than calling the helper
"full").
(*) Resolving the above confusion resulted in minor (one request)
differences in the number of helper requests queued by Squid for
external ACL, URL rewriting, and store ID helpers, with the adjusted
behavior [better] matching the documentation.
Amos Jeffries [Sat, 6 Aug 2016 04:49:55 +0000 (16:49 +1200)]
Fix clang errors with global InstanceId initialization
Move static member Last into change() method to avoid initialization order
errors when a caller uses a global InstanceId object before the library
instantiating its template is initialized.
Also, convert the Prefix static member to a constant string in a prefix()
method to avoid the same issue there.
Many web servers do not have complete certificate chains. Many browsers use
certificate extensions of the server certificate and download the missing
intermediate certificates automatically from the Internet.
This patch add this feature to Squid.
The information for missing issuer certificates provided by the Authority
Information Access X509 extension. This describes the format and the location
of additional information provided by the issuer of the certificate.
This patch:
- Implements a class Downloader as an independet AsyncJob class. This new
class can be used by internal squid subsystems to download objects from
the network.
- Modify Ssl::PeerConnector class to use new Downloader class to
retrieve missing certificates from the net. The URIs of missing
certificates from the Authority Information Access X509 extension.
- Implements a new basic certificates parser based on openSSL for the
TLS handshake messages parser.
- Modify the Ssl::ServerBio class to:
* Buffer the Server Hello message and not pass it to the openSSL library
until downloading missing certificates, if any, is finished.
* Extract server certificates from server hello message.
This is required to check if there are missing certificates, and if yes
give the chance to squid to download missing certificates and complete
certificate chains before pass them for processing to openSSL
TODO:
- Add support for certs-only CMS message.
From RFC 4325:
"Where the information is available via HTTP or FTP, accessLocation
MUST be a uniformResourceIdentifier and the URI MUST point to either
a single DER encoded certificate as specified in [RFC2585] or a
collection of certificates in a BER or DER encoded "certs-only" CMS
message as specified in [RFC2797]. "
...
"Conforming applications that support HTTP or FTP for accessing
certificates MUST be able to accept individual DER encoded
certificates and SHOULD be able to accept "certs-only" CMS messages."
Squid segfault via Ftp::Client::readControlReply().
Ftp::Client::scheduleReadControlReply(), which may called from the
asynchronous start() or readControlReply()/handleControlReply()
handlers, does not check whether the control connection is still usable
before using it.
Certain server certificates with a large chain and/or large certificates
(i.e. due to excessive amount of SAN entries) are producing helper requests and
replies which are larger than the 32KB limit set in src/helper.cc.
The major problem for squid is that the helper response should fit in the
helper read buffer. Currently squid starts with 4K request buffer and if
required may increase the buffer size up to 32K.
This patch:
- Uses a constant-size read buffer for helpers and accumulates the helper
response to Helper::Reply object.
- Changes the HelperServerBase::requests list to hold list of the new
Helper::Xaction class which holds pairs of Helper::Request and
Helper::Reply objects
- Modifies the Helper::Reply class to accumulate data and restricts the
required memory allocations
... also address Bug 4311 and partially address Bug 2833 and Bug 4471.
Extend collapsed_forwarding functionality to internal revalidation
requests. This implementation does not support Vary-controlled cache
objects and is limited to SMP-unaware caching environments, where each
Squid worker knows nothing about requests and caches handled by other
workers. However, it also lays critical groundwork for future SMP-aware
collapsed revalidation support.
Prior to these changes, multiple concurrent HTTP requests for the same
stale cached object always resulted in multiple internal revalidation
requests sent by Squid to the origin server. Those internal requests
were likely to result in multiple competing Squid cache updates, causing
cache misses and/or more internal revalidation requests, negating
collapsed forwarding savings.
Internal cache revalidation requests are collapsed if and only if
collapsed_forwarding is enabled. There is no option to control just
revalidation collapsing because there is no known use case for it.
* Public revalidation keys
Each Store entry has a unique key. Keys are used to find entries in the
Store (both already cached/swapped_out entries and not). Public keys are
normally tied to the request method and target URI. Same request
properties normally lead to the same public key, making cache hits
possible. If we were to calculate a public key for an internal
revalidation request, it would have been the same as the public key of
the stale cache entry being revalidated. Adding a revalidation response
to Store would have purged that being-revalidated cached entry, even if
the revalidation response itself was not cachable.
To avoid purging being-revalidated cached entries, the old code used
private keys for internal revalidation requests. Private keys are always
unique and cannot accidentally purge a public entry. On the other hand,
for concurrent [revalidation] requests to find the store entry to
collapse on, that store entry has to have a public key!
We resolved this conflict by adding "scope" to public keys:
* Regular/old public keys have default empty scope (that does not affect
key generation). The code not dealing with collapsed revalidation
continues to work as before. All entries stored in caches continue to
have the same keys (with an empty scope).
* When collapsed forwarding is enabled, collapsable internal
revalidation requests get public keys with a "revalidation" scope
(that is fed to the MD5 hash when the key is generated). Such a
revalidation request can find a matching store entry created by
another revalidation request (and collapse on it), but cannot clash
with the entry being revalidated (because that entry key is using a
different [empty] scope).
This change not only enables collapsing of internal revalidation
requests within one worker, but opens the way for SMP-aware workers to
share information about collapsed revalidation requests, similar to how
those workers already share information about being-swapped-out cache
entries.
After receiving the revalidation response, each collapsed revalidation
request may call updateOnNotModified() to update the stale entry [with
the same revalidation response!]. Concurrent entry updates would have
wasted many resources, especially for disk-cached entries that support
header updates, and may have purged being-revalidated entries due to
locking conflicts among updating transactions. To minimize these
problems, we adjusted header and entry metadata updating logic to skip
the update if nothing have changed since the last update.
Also fixed Bug 4311: Collapsed forwarding deadlocks for SMP Squids using
SMP-unaware caches. Collapsed transactions stalled without getting a
response because they were waiting for the shared "transients" table
updates. The table was created by Store::Controller but then abandoned (not
updated) by SMP-unaware caches. Now, the transients table is not created
at all unless SMP-aware caches are present. This fix should also address
complaints about shared memory being used for Squid instances without
SMP-aware caches.
A combination of SMP-aware and SMP-unaware caches is still not supported
and is likely to stall collapsed transactions if they are enabled. Note
that, by default, the memory cache is SMP-aware in SMP environments.
There are situations when Squid logs nothing to access.log after an
[abnormal] transaction termination. Such "stealthy" transactions may be
a security risk and an accounting problem.
ClientHttpRequest is responsible for logging most transactions but that
object is created only after the HTTP request headers are successfully
parsed. Request header parsing errors may be detected and logged
appropriately, but the job handling the incoming transaction may
terminate for reasons outside the parsing code control (e.g., a job-
killing exception thrown when there are no request headers to start
parsing yet or when the job waits for more request headers to finishing
parsing).
This change adds access logging for three cases:
1. accept(2) system call errors (before ConnStateData job is created).
2. Unexpected ConnStateData job termination, when there is no
ClientHttpRequest to log the failure.
3. Connections which send no bytes: these connections drain Squid
resources and, hence, should be logged.
TODO: make this behavior configurable because some browsers are known to
routinely create such connections(and, hence, logging them may create
too much noise in some environments).
* rename initializeSsl() method to initializeTls()
* use SessionPointer instead of SessionPtr
* return value is best used in boolean expressions, so make it bool
this also resolves some template issues with SessionPointer return type.
* rename the session parameter to serverSession which better documents what
it is than 'ssl', and distinguishes it from clientSession in child methods
which have to deal with both.
- Add/fix basic documentation for Downloader and DownloaderContext classes
- Move DownloaderContext class definition to Downloader.cc file
- fix cbdata leaks inside DownloaderCotnext destructor, caused by simple typo
mistake.
- Do not cbdatalock the ClientHttpRequest object inside DownloaderContext.
This class is responsible to hold the main pointer and finaly delete the
ClientHttpRequest object.
projects / thirdparty / squid.git / log
