<div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Alex,</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Thanks for your reply. I will try your patch.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">FWIW, we eventually arrived at a very different fix. See attached.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Best.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 10, 2023 at 2:09 PM Alex Rousskov <<a href="mailto:rousskov@measurement-factory.com">rousskov@measurement-factory.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 3/23/23 19:56, Hamilton Coutinho wrote:<br>
<br>
> We found a way to reproduce the leak: set up squid to run in intercept <br>
> mode + SSL description + memory pools off (to ease identifying the leak) <br>
> and then generate requests to sites with invalid certs (eg, CA not <br>
> installed), for instance: curl -k <a href="https://slscr.update.microsoft.com" rel="noreferrer" target="_blank">https://slscr.update.microsoft.com</a> <br>
<br>
Thanks a lot for sharing those details! Using them, I was able to <br>
reproduce one memory leak. Please try the following fix:<br>
<a href="https://github.com/squid-cache/squid/pull/1346" rel="noreferrer" target="_blank">https://github.com/squid-cache/squid/pull/1346</a><br>
<br>
I do not know whether that PR changes port to v5 cleanly or whether the <br>
same bug exists in v5. I can only speculate that v5 has a similar leak.<br>
<br>
<br>
> squidclient mgr:mem should show ever increasing HttpRequest instances.<br>
<br>
> As far as I can tell, the HttpRequest object created <br>
> in ConnStateData::buildFakeRequest() is never freed because its refcount <br>
> > 0.<br>
<br>
> Any ideas where an HTTPMSGUNLOCK() might be missing?<br>
<br>
Sorry, I do not know. In my master/v7-based tests, HttpRequest objects <br>
are not leaking. However, I am not testing an intercepting Squid. I <br>
still have one red flag to investigate, so perhaps I will be able to <br>
reproduce and fix that HttpRequest leak as well.<br>
<br>
<br>
HTH,<br>
<br>
Alex.<br>
<br>
<br>
> On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho wrote:<br>
> <br>
> Hi Alex,<br>
> <br>
> Thanks for the prompt reply! Thanks also for the clarifications.<br>
> <br>
> Agreed, I just realized the requests seem to be failing<br>
> with Http::scServiceUnavailable, so my focus turned<br>
> to Security::PeerConnector::sslCrtvdHandleReply() and friends.<br>
> <br>
> Best.<br>
> <br>
> On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov<br>
> <<a href="mailto:rousskov@measurement-factory.com" target="_blank">rousskov@measurement-factory.com</a><br>
> <mailto:<a href="mailto:rousskov@measurement-factory.com" target="_blank">rousskov@measurement-factory.com</a>>> wrote:<br>
> <br>
> On 1/18/23 13:46, Hamilton Coutinho wrote:<br>
> > Hi all,<br>
> ><br>
> > We are observing what seems to be several objects leaking in<br>
> the output<br>
> > mgr:mem, to the tune of 10s of 1000s<br>
> ><br>
> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail, cbdata PeekingPeerConnector (31), etc.<br>
> ><br>
> > We dumped a core and managed to find some HttpRequest objects<br>
> and they<br>
> > all seem to have failed in the same way, with an<br>
> ERR_SECURE_CONNECT_FAIL<br>
> > category, for a site that has a certificate signed by a CA<br>
> authority not<br>
> > available to squid.<br>
> ><br>
> > If I would guess, the origin of the problem might be in<br>
> > Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():<br>
> ><br>
> > if (finalAction == Ssl::bumpTerminate) {<br>
> > bail(new ErrorState(ERR_SECURE_CONNECT_FAIL,<br>
> Http::scForbidden,<br>
> > request.getRaw(), al));<br>
> > clientConn->close();<br>
> > clientConn = nullptr;<br>
> ><br>
> > Wondering if assigning null to clientConn there would be<br>
> premature.<br>
> <br>
> <br>
> FWIW, that connection pointer reset itself looks OK to me.<br>
> ConnStateData<br>
> and/or others should have a connection closure handler attached<br>
> to the<br>
> clientConn descriptor. That handler should be notified by Comm and<br>
> initiate cleanup of the objects responsible for client-Squid<br>
> communication.<br>
> <br>
> The bail() call above should inform the requestor about the<br>
> error/termination and terminate this AsyncJob. That requestor<br>
> should<br>
> then close the Squid-server connection and clean up associated<br>
> state.<br>
> <br>
> While there may be bugs in those "should..." sequences, please<br>
> note that<br>
> the pasted code is not related to handling of untrusted origin<br>
> servers<br>
> (unless your ssl_bump rules specifically activate the terminate<br>
> action<br>
> upon discovering such an origin server). The pasted code is<br>
> reacting to<br>
> an "ssl_bump terminate" rule matching.<br>
> <br>
> <br>
> Cheers,<br>
> <br>
> Alex.<br>
> <br>
> _______________________________________________<br>
> squid-dev mailing list<br>
> <a href="mailto:squid-dev@lists.squid-cache.org" target="_blank">squid-dev@lists.squid-cache.org</a><br>
> <mailto:<a href="mailto:squid-dev@lists.squid-cache.org" target="_blank">squid-dev@lists.squid-cache.org</a>><br>
> <a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a><br>
> <<a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a>><br>
> <br>
> <br>
> <br>
> -- <br>
> Hamilton<br>
> <br>
> <br>
> <br>
> -- <br>
> Hamilton<br>
> <br>
> _______________________________________________<br>
> squid-dev mailing list<br>
> <a href="mailto:squid-dev@lists.squid-cache.org" target="_blank">squid-dev@lists.squid-cache.org</a><br>
> <a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a><br>
<br>
_______________________________________________<br>
squid-dev mailing list<br>
<a href="mailto:squid-dev@lists.squid-cache.org" target="_blank">squid-dev@lists.squid-cache.org</a><br>
<a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a><br>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><font size="2">Hamilton</font></div></div></div></div></div>