[squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate

[Hamilton Coutinho]

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 <
rousskov at measurement-factory.com> 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
> >     <rousskov at measurement-factory.com
> >     <mailto:rousskov at measurement-factory.com>> 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
> >         squid-dev at lists.squid-cache.org
> >         <mailto:squid-dev at lists.squid-cache.org>
> >         http://lists.squid-cache.org/listinfo/squid-dev
> >         <http://lists.squid-cache.org/listinfo/squid-dev>
> >
> >
> >
> >     --
> >     Hamilton
> >
> >
> >
> > --
> > Hamilton
> >
> > _______________________________________________
> > squid-dev mailing list
> > squid-dev at lists.squid-cache.org
> > http://lists.squid-cache.org/listinfo/squid-dev
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>


-- 
Hamilton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20230510/3c75bcf6/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid.diff
Type: text/x-patch
Size: 1179 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20230510/3c75bcf6/attachment.bin>