squid
squid copied to clipboard
squid-cache
Bug 4587: socket/memory leak
In situations where the client has made a CONNECT request, if the ICAP server responds to the REQMOD by returning a new HTTP response, Squid logs "abandoning" and stops reading from the socket. Eventually the client drops the connection, which drops into the CLOSE_WAIT state. Since Squid never services the socket, it is never closed and stays in the CLOSE_WAIT state forever. Associated memory is also not freed, resulting in a significant memory leak.
Some situations where the ICAP server may want to return a new HTTP response for a CONNECT are:
- To forbid the connection by returning a 403 Forbidden.
- To demand authentication by returning a 407 Proxy Authorisation Required, in situations where the ICAP server, rather than Squid, is handling authentication.
This change does 3 things:
- If REQMOD responds with an HTTP 200 response, an error is logged since this is a bug.
- If REQMOD responds with any other HTTP response, set readMore so that Squid can process a new request after returning the HTTP response.
- If the "abandoning" state is reached, close the socket.
Bug #4587 also discusses a situation where the ICAP server modifies the HTTP request such that it is no longer a CONNECT. This change does not provide comprehensive support for doing that, but does at least ensure that the socket and memory are not leaked.
In the discussion regarding this bug, concerns were raised that the "abandoning" state may be reached for other unknown cases where closing a connection would kill a valid transaction. If that is a significant concern, that part of this change can be removed, however, I argue that:
- We have been running many systems in the wild since writing the original (much less complete) patch attached to the bug report over 5 years ago, and have never knowlingly had a problem with valid transactions ending up in the "abandoning" state.
- It is surely better to occasionally kill an extremely rare valid transaction than to leak memory and sockets until the entire proxy crashes.
I agree that it would be nice to properly characterise what transactions may end up in the "abandoning" state, but until that is done I feel that closing those connections is the lesser of the two evils.
