Hi Alex,
Thanks for your reply. I will try your patch.
FWIW, we eventually arrived at a very different fix. See attached.
Best.
On Wed, May 10, 2023 at 2:09 PM Alex Rousskov <
[email protected]> wrote:
> On 3/23/23 19:56, Hamilton Coutinho wrote:
>
> > We found a way to reproduce the leak: set up squid to run in intercept
> > mode + SSL description + memory pools off (to ease identifying the leak)
> > and then generate requests to sites with invalid certs (eg, CA not
> > installed), for instance: curl -k https://slscr.update.microsoft.com
>
> Thanks a lot for sharing those details! Using them, I was able to
> reproduce one memory leak. Please try the following fix:
> https://github.com/squid-cache/squid/pull/1346
>
> I do not know whether that PR changes port to v5 cleanly or whether the
> same bug exists in v5. I can only speculate that v5 has a similar leak.
>
>
> > squidclient mgr:mem should show ever increasing HttpRequest instances.
>
> > As far as I can tell, the HttpRequest object created
> > in ConnStateData::buildFakeRequest() is never freed because its refcount
> > > 0.
>
> > Any ideas where an HTTPMSGUNLOCK() might be missing?
>
> Sorry, I do not know. In my master/v7-based tests, HttpRequest objects
> are not leaking. However, I am not testing an intercepting Squid. I
> still have one red flag to investigate, so perhaps I will be able to
> reproduce and fix that HttpRequest leak as well.
>
>
> HTH,
>
> Alex.
>
>
> > On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho wrote:
> >
> > Hi Alex,
> >
> > Thanks for the prompt reply! Thanks also for the clarifications.
> >
> > Agreed, I just realized the requests seem to be failing
> > with Http::scServiceUnavailable, so my focus turned
> > to Security::PeerConnector::sslCrtvdHandleReply() and friends.
> >
> > Best.
> >
> > On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov
> > <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > On 1/18/23 13:46, Hamilton Coutinho wrote:
> > > Hi all,
> > >
> > > We are observing what seems to be several objects leaking in
> > the output
> > > mgr:mem, to the tune of 10s of 1000s
> > >
> >
> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail,
> cbdata
> PeekingPeerConnector (31), etc.
> > >
> > > We dumped a core and managed to find some HttpRequest objects
> > and they
> > > all seem to have failed in the same way, with an
> > ERR_SECURE_CONNECT_FAIL
> > > category, for a site that has a certificate signed by a CA
> > authority not
> > > available to squid.
> > >
> > > If I would guess, the origin of the problem might be in
> > > Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():
> > >
> > > if (finalAction == Ssl::bumpTerminate) {
> > > bail(new ErrorState(ERR_SECURE_CONNECT_FAIL,
> > Http::scForbidden,
> > > request.getRaw(), al));
> > > clientConn->close();
> > > clientConn = nullptr;
> > >
> > > Wondering if assigning null to clientConn there would be
> > premature.
> >
> >
> > FWIW, that connection pointer reset itself looks OK to me.
> > ConnStateData
> > and/or others should have a connection closure handler attached
> > to the
> > clientConn descriptor. That handler should be notified by Comm
> and
> > initiate cleanup of the objects responsible for client-Squid
> > communication.
> >
> > The bail() call above should inform the requestor about the
> > error/termination and terminate this AsyncJob. That requestor
> > should
> > then close the Squid-server connection and clean up associated
> > state.
> >
> > While there may be bugs in those "should..." sequences, please
> > note that
> > the pasted code is not related to handling of untrusted origin
> > servers
> > (unless your ssl_bump rules specifically activate the terminate
> > action
> > upon discovering such an origin server). The pasted code is
> > reacting to
> > an "ssl_bump terminate" rule matching.
> >
> >
> > Cheers,
> >
> > Alex.
> >
> > _______________________________________________
> > squid-dev mailing list
> > [email protected]
> > <mailto:[email protected]>
> > http://lists.squid-cache.org/listinfo/squid-dev
> > <http://lists.squid-cache.org/listinfo/squid-dev>
> >
> >
> >
> > --
> > Hamilton
> >
> >
> >
> > --
> > Hamilton
> >
> > _______________________________________________
> > 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
>
--
Hamilton
diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index 494edabb8..75a59a5f1 100755
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -534,6 +534,13 @@ Security::PeerConnector::disconnect()
serverConn = nullptr;
}
+void
+Security::PeerConnector::negotiationFinished()
+{
+ debugs(83, 7, this);
+ Must(done());
+}
+
void
Security::PeerConnector::callBack()
{
@@ -544,6 +551,9 @@ Security::PeerConnector::callBack()
// to call back holds.
callback = NULL; // this should make done() true
ScheduleCallHere(cb);
+
+ // Forces another scheduling so we get eventually deleted.
+ CallJobHere(83, 7, CbcPointer<PeerConnector>(this), PeerConnector, negotiationFinished);
}
void
diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h
index 10700d796..03f4be2ab 100755
--- a/src/security/PeerConnector.h
+++ b/src/security/PeerConnector.h
@@ -66,6 +66,8 @@ public:
const time_t timeout = 0);
virtual ~PeerConnector();
+ void negotiationFinished();
+
/// hack: whether the connection requires fwdPconnPool->noteUses()
bool noteFwdPconnUse;
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
