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

[Alex Rousskov]

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