On 10/28/2016 01:11 PM, Amos Jeffries wrote:
On 21/10/2016 3:55 a.m., Christos Tsantilas wrote:
Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.
Use case: Skype groups appear to use TLS-encrypted MSNP protocol instead
of HTTPS. This change allows Squid admins using SslBump to tunnel Skype
groups and similar non-HTTP traffic bytes via "on_unsupported_protocol
tunnel all". Previously, the combination resulted in encrypted HTTP 400
(Bad Request) messages sent to the client (that does not speak HTTP).
Also this patch:
* fixes bug 4529: !EBIT_TEST(entry->flags, ENTRY_FWD_HDR_WAIT)
assertion in FwdState.cc.
* when splicing transparent connections during SslBump step1, avoid
access-logging an extra record and log %ssl::bump_mode as the expected
"splice" not "none".
* handles an XXX comment inside clientTunnelOnError for possible memory
leak of client streams related objects
* fixes TunnelStateData logging in the case of splicing after peek.
This is a Measurement Factory project.
Are any of these additional fixes able to be easily broken out into
separate patches? It would greatly help the auditing process to get
smaller patches.
- It looks like the chunk in ConnStateData::clientParseRequests() is a
leak fix which could go in separately (eg right now if thats correct).
If you refer to Http::StreamPointer use instead of a raw Http::Stream,
this is because after the latest changes the context (the Http::Stream
object), can be deleted somewhere inside processParsedRequest().
It is not trying to fix a leak. This code required because
clientTunnelOnError() may destroy the Http::Stream object.
But my sense is that the doCallouts (called somewhere inside
processParsedRequest/clientProcessRequest) may result to Http::Stream
object, so probably this part of patch fixes other bugs too.
Personally I prefer to keep this patch as one patch, it is not easy to
split it. However if you insist I will apply as separate patch the
ConnStateData::clientParseRequests() changes plus the other Http::Stream
raw pointers to Http:streamPointer conversion..
in src/FwdState.cc:
* Most of the changed lines are a needless de-scoping of code.
- note that the sub-scope existed to begin with so as to silence noisy
false-positives by static analysis tools (trunk rev.14790).
* The one meaningful change is erasing
"pinned_connection->stopPinnedConnectionMonitoring()". Seemingly to move
it to tunnel.cc
- are comm_add_close_handler() and syncWithServerConn() safe to run on
a still-monitored FD ?
- what happens to all the pinned connection uses that do not lead to
tunnel? eg connection auth?
This method is removed because it is needles. It has already called from
inside pinned_connection->borrowPinnedConnection() call, some lines before.
The patch does not actually change anything important in these lines,
just cleanup the code.
Please use nullptr in new code instead of NULL.
- there is one part "if (cause != NULL && cause->method == " that
should either not have the " != NULL" bytes, or use nullptr explicitly.
- I prefer the former, "if (cause && cause->method == ", when testing
pointers existence-vs-nil.
in tunnelPeerSelectComplete()
- "if (!bail &&" can be replaced by "else if (".
- please take advantage of the surrounding code being re-written to
cleanup:
if (peer_paths == NULL || peer_paths->size() < 1) {
debugs(26, 3, HERE << "No paths found. Aborting CONNECT");
as:
if (!peer_paths || peer_paths->size() < 1) {
debugs(26, 3, "No paths found. Aborting CONNECT");
Amos
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
