Alex Rousskov [Mon, 18 Apr 2016 19:08:25 +0000 (13:08 -0600)]
Stop parsing response prefix after discovering an "HTTP/0.9" response.
Otherwise, our "X-Transformed-From: HTTP/0.9" headers are going to
be ignored, and the rest of the received bytes are going to be parsed
(and modified!) as an HTTP/1 response header, followed by message body.
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.
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.
- Throw a TextException on errors in Ssl::SSL_add_untrusted_cert
- Move most of the code from ConnStateData::start() into the new virtual method
ConnStateData::prepUserConnection. Call this method instead calling
grandparents ::start() method from Downloader::start().
Also implement an empty Downloader::prepUserConnection() method
- Fix Downloader::isOpen() to use doneAll() to check if its jobs is finished
and its job assumed as closed.
- Fix Downloader::start to handle the case a wrong HTTP request passed from
the user (eg malformed URL). In this case calaback to the user with an
Http::scInternalServerError.
- Remove tbe Downloader::callException() method, existed only to print a
debug message. Add a debug message in AsyncJob::callException method instead.
- Replaces NULL with nullptr
- Fixes debug messages and comments
It did not catch all cases of the SP it was intended to and the chunked
encoding parse will need significantly different changes pending IETF WG
discussions.
Keep the violationLevel() member, which will be useful in general.
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.
Initially, they looked like we were [incorrectly] subtracting -1 from
some buffer length (read -1, pass 5; read -1, pass 4). Now, I believe
they [just] indicated unnecessary network reads. The fixed sequence
looks similar to this (note no network reads):
| bio.cc(289) giveBuffered: Pass 5 read bytes to openSSL
| bio.cc(289) giveBuffered: Pass 4 read bytes to openSSL
The refactored ServerBio code starts in "parsing" state (SSL Hello
parsing is the primary ServerBio functionality). Only when that parsing
is over, ServerBio starts feeding OpenSSL with received bytes. This
internal logic allows us to hide parsing from callers and avoid the
confusing public holdRead API.
AFAICT, the default copy constructor should work and the removed
explicit constructor was not copying the staticContext member, for no
documented reason (that I could find). It was also unused [in my tests].
If the partial copy constructor was abused for something useful, then a
different approach should be found -- the one that does not violate the
standard copy constructor post-conditions.
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).
Record SSL client and SSL server details (supported TLS version/requested
TLS version) for fast-sni branch
Currently the fast-SNI-peek branch does not parse SSL client hello in
Client-First and Server-First bumping modes. This patch always "peeks" the
SSL client hello message and apply the squid TLS parser for Client-First
and Server-First modes to.
Also this patch moves the code which retrieves SSL server details from
PeekingPeerConnector class to PeerConnector class t retrieve details for all
SSL server side connections.
It is possible that the connection will be closed somewhere inside
"clientTunnelOnError" call, inside ConnStateData::fakeAConnectRequest which
is called by ConnStateData::clientTunnelOnError or inside spliceOnError()
while trying to splice(). In this case the callers should be informed to abort
imediatelly, but instead continues, and try to set timeout handler on closed
connection.
This patch:
- Modify ConnStateData::fakeAConnectRequest and ConnStateData::splice methods to return boolean and false on error.
- Does not close the connection inside ConnStateData::fakeAConnectRequest but
instead return false and allow callers to close the connection if required.
Alex Rousskov [Thu, 7 Apr 2016 00:08:06 +0000 (18:08 -0600)]
Fixed 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.
- remove uneeded members from classes
- Remove the TlsDetails object from ClientBio
- Enable SSL Server Hello parsing code
- remove uneeded code
- Fix bump client first and bump server first bumping modes
author: Nathan Hoad <nathan@getoffmalawn.com>
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 parses incomming client SSL hello mesage, before create any openSSL
related structures and objects. After acl check at bumping step2.
Actually creating openSSL objects for client side still can be delayed
untill the server side is finishes. The only reason to create openSSL
structures imediatelly after step2 is to use openSSL to check for unsupported
comunications features and settings and fallback to spliceOnError.
Regression:
Squid does not parses client Hello message in the case of bump-server-first
and bump-client-first.
The supported and requested SSL versions (i %ssl::>received_supported_version
and %ssl::>received_hello_version formating codes ) can not be logged for
these modes.
Improve TLS/SSL parsing code in Handshale.cc and use it inside bio.cc for client
and server messages
- full implementation for TLS and SSLv2 parsers inside Handshake.cc/h files
- remove parsing code from bio.cc
- Store parsed info in new Security::TlsDetails struct and remove the
Ssl::sslFeatures class
- improve SSLv2 parsing code.
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).
Nathan Hoad [Fri, 25 Mar 2016 13:03:30 +0000 (02:03 +1300)]
Fix memory leak of AccessLogentry::url
... created by ACLFilledChecklist::syncAle().
::syncAle() is the only place in the codebase that assigns a URL that
AccessLogEntry is expected to free(), which AccessLogEntry doesn't do.
This results in a memory leak.
Alex Rousskov [Thu, 24 Mar 2016 17:02:25 +0000 (11:02 -0600)]
Added shared_memory_locking configuration directive to control mlock(2).
Locking shared memory at startup avoids SIGBUS crashes when kernel runs
out of RAM during runtime. Why not enable it by default? Unfortunately,
locking requires privileges and/or much-higher-than-default
RLIMIT_MEMLOCK limits. Thus, requiring locked memory by default is
likely to cause too many complaints, especially since Squid has not
required that before. The default is off, at least for now.
As we gain more experience, we may try to enable locking by default
while making default locking failures non-fatal and warning about
significant [accumulated] locking delays.
Move BinaryTokenizer, Ssl::HandshakeParser and Ssl::Rfc5246::* classes to
their own *.cc,*.h files
-Move the BinaryTokenizer from ssl/bio.* files to parser/BinaryTokenizer.{cc,h}
-Move the Ssl::Handshake and related structures and declarations from ssl/bio.*
to security/Handshake.{c,h}, and under the Security namespace
(Security::Handshake)
-Move the Ssl::Rfc5246::* classes from ssl/bio.* to security/Handshake.{c,h},
under the Security namespace.
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.
Author: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Added ACL-driven server_pconn_for_nonretriable squid.conf directive.
This directive provides fine-grained control over persistent connection
reuse when forwarding HTTP requests that Squid cannot retry. It is
useful in environments where opening new connections is very expensive
and race conditions associated with persistent connections are very rare
and/or only cause minor problems.
Alex Rousskov [Sat, 12 Mar 2016 18:40:29 +0000 (11:40 -0700)]
Trying to avoid "looser throw specifier" error with Wheezy GCC.
AFAICT, the default CbdataParent destructor gets implicit
"noexcept(true)" specifier (because the default destructor does not
throw itself, and CbdataParent has no data members or parents that could
have contributed potentially throwing destructors). The AsyncJob child
uses a lot of things that might throw during destruction (the compiler
cannot tell for sure because we do not use noexcept specifiers). Thus,
the compiler has to use "noexcept(false)" specifier for ~AsyncJob, which
is "looser" that "noexcept(true)" for ~CbdataParent and, hence, violates
the parent interface AsyncJob is implementing/overriding.
I have doubts about the above analysis because many other compilers,
including GCC v5 and clang are happy with the default virtual
CbdataParent destructor. If my analysis is correct, then the rule of
thumb is: Base classes must not use "= default" destructors until all
our implicit destructors become "noexcept".
projects / thirdparty / squid.git / log
