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

[Alex Rousskov]

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.