Re: [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 <
rouss...@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
> >  > > 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@lists.squid-cache.org
> > 
> > http://lists.squid-cache.org/listinfo/squid-dev
> > 
> >
> >
> >
> > --
> > Hamilton
> >
> >
> >
> > --
> > Hamilton
> >
> > ___
> > squid-dev mailing list
> > squid-dev@lists.squid-cache.org
> > http://lists.squid-cache.org/listinfo/squid-dev
>
> 

Re: [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
mailto:rouss...@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@lists.squid-cache.org

http://lists.squid-cache.org/listinfo/squid-dev




-- 
Hamilton




--
Hamilton

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev