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

[Hamilton Coutinho]

Hi all,

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.
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?

Thanks!

On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho <
hamilton.coutinho at gmail.com> 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> 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
>> http://lists.squid-cache.org/listinfo/squid-dev
>>
>
>
> --
> Hamilton
>


-- 
Hamilton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20230323/24f5727d/attachment.htm>