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.
The Ftp::Server::stopWaitingForOrigin() notification may come after
Ftp::Server (or an external force) has started closing the control
connection but before the Ftp::Server job became unreachable for
notifications. Writing a response in this state leads to assertions.
Other, currently unknown paths may lead to the same write-after-close
problems. This change protects all asynchronous notification methods
(except the connection closure handler itself) from exposing underlying
code to a closing control connection. This is very similar to checking
for ERR_CLOSING in Comm handlers.
Alex Rousskov [Thu, 30 Jun 2016 20:51:54 +0000 (08:51 +1200)]
Do not make bogus recvmsg(2) calls when closing UDS sockets.
comm_empty_os_read_buffers() assumes that all non-blocking
FD_READ_METHODs can read into an opaque buffer filled with random
characters. That assumption is wrong for UDS sockets that require an
initialized msghdr structure. Feeding random data to recvmsg(2) leads to
confusing errors, at best. Squid does not log those errors, but they
are visible in, for example, strace:
recvmsg(17, 0x7fffbb, MSG_DONTWAIT) = -1 EMSGSIZE (Message too long)
comm_empty_os_read_buffers() is meant to prevent TCP RST packets. The
function now ignores UDS sockets that are not used for TCP.
TODO: Useless reads may also exist for UDP and some TCP sockets.
This change fixes logic bugs that mostly affect performance: In micro-
tests, this change gives 10% performance improvement.
maybeMakeSpaceAvailable() is called with an essentially random in.buf.
The method must prepare in.buf for the next network read. The old code
was not doing that [well enough], leading to performance problems.
In some environments, in.buf often ends up having tiny space exceeding 2
bytes (e.g., 6 bytes). This happens, for example, when Squid creates and
parses a fake CONNECT request. The old code often left such tiny in.bufs
"as is" because we tried to ensure that we have at least 2 bytes to read
instead of trying to provide a reasonable number of buffer space for the
next network read. Tiny buffers naturally result in tiny network reads,
which are very inefficient, especially for non-incremental parsers.
I have removed the explicit "2 byte" space checks: Both the new and the
old code do not _guarantee_ that at least 2 bytes of buffer space are
always available, and the caller does not check that condition either.
If some other code relies on it, more fixes will be needed (but this
change is not breaking that guarantee -- either it was broken earlier or
was never fully enforced). In practice, only buffers approaching
Config.maxRequestBufferSize limit may violate this guarantee AFAICT, and
those buffers ought to be rare, so the bug, if any, remains unnoticed.
Another subtle maybeMakeSpaceAvailable() problem was that the code
contained its own buffer capacity increase algorithm (n^2 growth).
However, increasing buffer capacity exponentially does not make much
sense because network read sizes are not going to increase
exponentially. Also, memAllocStringmemAllocate() overwrites n^2 growth
with its own logic. Besides, it is buffer _space_, not the total
capacity that should be increased. More work is needed to better match
Squid buffer size for from-user network reads with the TCP stack buffers
and traffic patterns.
Both the old and the new code reallocate in.buf MemBlobs. However, the
new code leaves "reallocate or memmove" decision to the new
SBuf::reserve(), opening the possibility for future memmove
optimizations that SBuf/MemBlob do not currently support.
It is probably wrong that in.buf points to an essentially random MemBlob
outside ConnStateData control but this change does not attempt to fix that.
Alex Rousskov [Wed, 15 Jun 2016 22:08:16 +0000 (10:08 +1200)]
Do not allow low-level debugging to hide important/critical messages.
Removed debugs() side effects that inadvertently resulted in some
important/critical messages logged at the wrong debugging level and,
hence, becoming invisible to the admin. The removed side effects set the
"current" debugging level when a debugs() parameter called a function
that also called debugs(). The last nested debugs() called affected the
level of all debugs() still in progress!
Related changes:
* Reentrant debugging messages no longer clobber parent messages. Each
debugging message is logged separately, in the natural order of
debugs() calls that would have happened if debugs() were a function
(that gets already evaluated arguments) and not a macro (that
evaluates its arguments in the middle of the call). This order is
"natural" because good macros work like functions from the caller
point of view.
* Assertions hit while evaluating debugs() parameters are now logged
instead of being lost with the being-built debugs() log line.
* 10-20% faster debugs() performance because we no longer allocate a new
std::ostringstream buffer for the vast majority of debugs() calls.
Only reentrant calls get a new buffer.
* Removed old_debug(), addressing an old "needs to die" to-do.
* Removed do_debug() that changed debugging level while testing whether
debugging is needed. Use side-effect-free Debug::Enabled() instead.
Also removed the OutStream wrapper class. The wrapper was added in trunk
revision 13767 that promised to (but did not?) MemPool the debug output
buffers. We no longer "new" the buffer stream so a custom new() method
would be unused. Besides, the r13767 explanation implied that providing
a Child::new() method would somehow overwrite Parent::allocator_type,
which did not compute for me. Finally, Squid "new"s other allocator-
enabled STL objects without overriding their new methods so either the
same problem is still there or it did not exist (or was different?).
Also removed Debug::xassert() because the debugs() assertions now work
OK without that hack.
Alex Rousskov [Sat, 21 May 2016 15:52:02 +0000 (03:52 +1200)]
Fix icons loading speed.
Since trunk r14100 (Bug 3875: bad mimeLoadIconFile error handling), each
icon was read from disk and written to Store one character at a time. I
did not measure startup delays in production, but in debugging runs,
fixing this bug sped up icons loading from 1 minute to 4 seconds.
Steve Hill [Tue, 17 May 2016 14:58:50 +0000 (02:58 +1200)]
Support unified EUI format code in external_acl_type
Squid supports %>eui as a logformat specifier, which produces an EUI-48
for IPv4 clients and an EUI-64 for IPv6 clients. However, This is not
allowed as a format specifier for the external ACLs, and you have to use
%SRCEUI48 and %SRCEUI64 instead. %SRCEUI48 is only useful for IPv4
clients and %SRCEUI64 is only useful for IPv6 clients, so supporting
both v4 and v6 is a bit messy.
Adds the %>eui specifier for external ACLs and behaves in the same way
as the logformat specifier.
Amos Jeffries [Fri, 6 May 2016 09:39:48 +0000 (21:39 +1200)]
Fix SIGSEGV in ESIContext response handling
HttpReply pointer was being unlocked without heving been locked.
Resulting in a double-free. Make it use RefCount instead of
manual locking to ensure locked/unlock is always symmetrical.
Nathan Hoad [Fri, 6 May 2016 08:15:36 +0000 (20:15 +1200)]
Prevent Squid forcing -b 2048 into the arguments for sslcrtd_program
Previously Squid assumed it was running with the default sslcrtd_program, which
takes an argument for the FS block size. This causes issues for administrators
that use their own helpers that happen to take a -b argument that means
something else entirely, causing confusion and preventing them from removing
this argument.
A summary of the changes:
* Move the block size retrieval from Squid into security_file_certgen. It
does not use fsBlockSize as that introduces a lot of dependencies on
unrelated Squid code, e.g. fde, Debug, MemBuf.
* Make the -b argument mostly redundant, but leave it there so
administrators can overrule xstatvfs.
* Fix a small typo.
This work is submitted on behalf of Bloomberg L.P.
Squid currently does not handle SSL server responses that start with
an SSL Alert Record. Squid fails to parse the Server Hello message and
also fails to detect and handle resuming sessions.
Amos Jeffries [Mon, 2 May 2016 10:51:18 +0000 (22:51 +1200)]
Bug 4501: HTTP/1.1: normalize Host header
When absolute-URI is provided Host header should be ignored. However some
code still uses Host directly so normalize it using the URL authority
value before doing any further request processing.
For now preserve the case where Host is completely absent. That matters
to the CVE-2009-0801 protection.
This also has the desirable side effect of removing multiple or duplicate
Host header entries, and invalid port values.
cachemgr.cgi: use dynamic MemBuf for internal content generation
Using a fixed size buffer limits how big content lines can be. Modern
HTTP is fast reaching the point where such limits are problematic.
Also fixes incorrect uses of snprintf() by removing them.
Add chained certificates and signing certificate to peek-then-bumped connections.
The scenario this patch addresses is when Squid is configured with an
intermediate signing CA certificate, and clients have the root CA installed on
their machines. What happens is that the generated certificates come down with
an unknown issuer (the intermediate signing certificate), with no
intermediates, so they are rejected. By adding the configured certificate chain
as old client-first mode did, the intermediate and root certificates come down
as well, resulting in the issuer being identified and the connection being
established "securely".
This work is submitted on behalf of Bloomberg L.P.
Squid crashes on startup when the parent process exit()s after fork()ing
the kid process. Squid may also crash on shutdown after exiting main().
In both cases, the crashes are build- and environment-specific. Many
environments show no problems at all. Even disabling compiler
optimizations may prevent crashes. When crashes do happen, their
symptoms (e.g., backtrace) point to problems during destruction of
global objects, but the details point to innocent objects (e.g., PortCfg
or SSL_CTX).
In some environments, the following malloc error is printed on console
before the crash: "corrupted double-linked list".
This change replaces two StatHist globals used for SBuf statistics
collection with always-available singletons. The replaced globals could
be destructed before the last SBuf object using them, leading to memory
corruption (that would eventually crash Squid).
We should not expect the exact match because, as discovered during bug
3805 (r13947) fix, shared Segment::size() may exceed the originally
requested RAM amount due to stat() page rounding done by OSes like OS X.
Unfortunately, this rounding weakens the failed consistency check a lot.
TODO: Store the exact requested size and check against that as well.
Some servers cause an SSL handshake error with peek and splice.
The problem is related to the TLS Session Tickets extension handling. Squid
expects always a TLS Session Tickets extension, included in server hello
message, to assume that the ticket accepted and the session is a resumed
session, which is not always true.
Alex Rousskov [Tue, 12 Apr 2016 06:04:23 +0000 (18:04 +1200)]
Bug 4468: NotNode (!acl) naming: Terminate the name before strncat(name).
The fix may reduce or even eliminate garbage in logged ACL names (at
least). The bug was exposed by valgrind's "Conditional jump or move
depends on uninitialised value(s)" error.
However, pinger only drops setuid/setgid, and won't drop capabilities
after sockets are opened (when it is setuid-root, setuid(getuid()) also
drops capabilities, no code changes necessary; however, if it is only
setcap'ed, setuid() is no-op).
Fix is minimally tested, seems to work fine with both/either `setcap`
and `chmod u+s`; non-linux/non-libcap configurations should not be
affected).
Amos Jeffries [Wed, 23 Mar 2016 14:00:51 +0000 (03:00 +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.
Bug description:
- The client side and server side are finished
- On server side the Ftp::Relay::finalizeDataDownload() is called and
schedules the Ftp::Server::originDataCompletionCheckpoint
- On client side the "Ftp::Server::userDataCompletionCheckpoint" is
called. This is schedules a write to control connection and closes
data connection.
- The Ftp::Server::originDataCompletionCheckpoint is called which is
trying to write to control connection and the assertion triggered.
This bug is an corner case, where the client-side (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job before
respond to the FTP client. In this bug the existing mechanism, designed
to handle such problems, did not worked correctly and resulted to a double
write response to the client.
This patch try to fix the existing mechanism as follows:
- When Ftp::Server receives a "startWaitingForOrigin" callback, postpones
writting possible responses to the client and keeps waiting for the
stopWaitingForOrigin callback
- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
resumes any postponed response.
- When the Ftp::Client starts working on a DATA-related transaction, calls the
Ftp::Server::startWaitingForOrigin callback
- When the Ftp::Client finishes its job or when its abort abnormaly, checks
whether it needs to call Ftp::Server::stopWaitingForOrigin callback.
- Also this patch try to fix the status code returned to the FTP client
taking in account the status code returned by FTP server. The
"Ftp::Server::stopWaitingForOrigin" is used to pass the returned status code
to the client side.
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.
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.
William Lima [Tue, 23 Feb 2016 15:47:10 +0000 (04:47 +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.
Alex Rousskov [Thu, 18 Feb 2016 04:15:33 +0000 (21:15 -0700)]
Better handling of huge response headers. Fewer "BUG 3279" messages.
When we failed to parse a response, do not store the fake half-baked
response (via a replaceHttpReply() call). Doing so leads to misleading
"BUG 3279: HTTP reply without Date" messages (at best). The fake
response is only meant for continueAfterParsingHeader().
Also removed a misleading XXX that may have caused Bug 4432 in v4.0
(trunk r14548).
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.
Markus Mayer [Sun, 31 Jan 2016 05:43:08 +0000 (18:43 +1300)]
Fix handling of shared memory left over by Squid crashes or bugs
A Squid instance may inherit an old shared memory segment from the
previous instance as the result of either a Squid crash or an at-exit
cleanup bug. This change fixes two problems triggered by old segments:
1. After an earlier OS X fix (bug 3805; trunk r13947), Squid stopped
initializing previously used shared memory. Uninitialzed memory
resulted in subtle bugs and crashes.
2. When called for an old Squid shared memory segment, OS X
ftruncate() fails with EINVAL, preventing Squid from starting when
the old segment is still around.
More specifically: Darwin ftruncate() calls pshm_truncate().
pshm_truncate() checks if the PSHM_ALLOCATED flag is already set on
the memory region. If the flag is set, the call fails with EINVAL.
Otherwise, pshm_truncate() sets PSHM_ALLOCATED for the region.
Since Squid must call ftruncate() to size every new segment, all
old Squid segments have that flag set, preventing ftruncate() calls
for old segments in newer Sqid instances from succeeding.
Fix invalid FTP connection handling on blocked content
FTP client gets stuck after the following chain of events:
* Client requests a file that will be blocked by ICAP.
* Squid starts downloading the file from the FTP server
and sends "150 Opening..." to the FTP client.
* Squid aborts the data connection with the FTP server
as soon as the ICAP service blocks it.
* Squid sends "451 Forbidden" to the FTP client.
* The FTP server sends "500 OOPS: setsockopt: linger" to Squid.
* Squid terminates the control connection to the FTP server.
* Squid establishes a new control connection to the FTP server
but does not authenticate itself.
* Further commands from the FTP client do not work any more.
The above and many similar problems exist because Squid handles
FTP client-to-squid and squid-to-FTP server data connections
independently from each other. In many cases, one connection does
not get notified about the problems with the other connection.
This patch:
- Add Ftp::MasterState::userDataDone to record received
the FTP client final response status code to sent (or to be send)
to the client.
- The Ftp::MasterState::waitForOriginData flag to hold status of the
squid-to-server side. If the squid-to-server side is not finishes
yet this is true.
- Send a control reply to the FTP client only after the data transfered
on both server and client sides.
- Split Client::abortTransaction to Client::abortOnData and to
Client::abortAll()
- Implement the Ftp::Relay::abortOnData() and Ftp::Relay::Abort()
(i.e., StoreEntry abort handler) to avoid closing the control
connection when the data connection is closed unexpectedly.
projects / thirdparty / squid.git / log
