Re: Squid 5.6 leaking memory when peeking for an origin with an invalid certificate

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

_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev

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;