[squid-dev] File descriptor leak at ICAP reqmod rewrites of CONNECT requests

[Alexey Sergin]

>> Could you please give me some advice on a better/proper fix, if close()
>> at "abandoning" time is wrong?
>
> Unfortunately, I cannot.

In fact, your message was very helpful! Thank you.


> In general, responding with a 403 does not invalidate the client
> connection, so it does not have to be closed. Said that, I am sure there
> are use cases where such closure would be a good idea. I would not be
> surprised if Squid closes the connection after denying regular requests.

Yes, rfc7231 states that:
   Any 2xx (Successful) response indicates that the sender (and all
   inbound proxies) will switch to tunnel mode immediately after the
   blank line that concludes the successful response's header section;
   data received after that blank line is from the server identified by
   the request-target.  Any response other than a successful response
   indicates that the tunnel has not yet been formed and that the
   connection remains governed by HTTP.

In case of non-2xx http response from icap reqmod, the client connection
should continue to function in http mode, so there is no need to close it.

In case of 2xx http response from icap reqmod, Squid will not do any
tunneling stuff for this CONNECT request, and thus it would be better to
close the client connection. However I am not aware of any reasonable
scenario where icap (reqmod) server would rewrite a CONNECT request with
2xx http response.


> What is currently missing (at least) is understanding of what is going
> on. The correct fix, whatever it is, would be determined by that
> understanding.

The reason ConnStateData::kick() gets into the "abandoning" branch is due
to a combination of the following circumstances:

1. Function clientProcessRequest() executes this code:
    // Let tunneling code be fully responsible for CONNECT requests
    if (http->request->method == Http::METHOD_CONNECT) {
        context->mayUseConnection(true);
        conn->flags.readMore = false;
    }

2. No tunneling is actually set up in case of icap reqmod rewrite of
CONNECT request with http response;

3. Conditions for ConnStateData::kick() "abandoning" branch are satisfied
after the processing of CONNECT request finishes.


ACL deny of CONNECT request could trigger the same "abandoning" conditions,
but it does not, because:

1. ClientRequestContext::clientAccessCheckDone(ACCESS_DENIED) sets
ClientRequestContext::readNextRequest and ClientRequestContext::error
variables;

2. ClientHttpRequest::doCallouts() checks those variables and executes:
    getConn()->flags.readMore = true; // resume any pipeline reads.


I tried to apply the same approach (as in the ACL) for the case of ICAP
REQMOD, please see attached patch.
I've done limited testing and the patch seems to fix the problem.

Is ClientHttpRequest::handleAdaptedHeader() time suitable for re-enabling
connection readMore flag and for connection stopReceiving()?
Would it be better to postpone the re-enabling /  to a later stage
stopReceiving()?
Maybe to ClientHttpRequest::endRequestSatisfaction() ?


Thanks!


On Fri, Dec 11, 2020 at 6:40 PM Alex Rousskov <
rousskov at measurement-factory.com> wrote:

> On 12/10/20 3:33 PM, Alexey Sergin wrote:
>
> > - Squid writes to cache.log a message like "kick abandoning <....>";
>
> These messages indicate a Squid bug, most likely in REQMOD request
> satisfaction implementation specific to CONNECT use cases. The messages
> are not prefixed with a "BUG" label, but they should be.
>
>
> > - Squid does not close the file descriptor used for http client
> connection.
>
> Yes, that is a likely side effect of the above-mentioned bug.
>
>
> > client_side.cc. Closing clientConnection right after
> > "debugs(<....>abandoning<....>)" fixes the leak.
>
> > Is it ok to always close() clientConnection when "abandoning" thing
> > happens?
>
> > Are there any known scenarios where this close() would be
> > inappropriate?
>
> Unknown. Such a closure (alone) is not a proper fix. If well-tested, it
> may be considered to be a good-enough workaround, but no more than that.
>
> What is currently missing (at least) is understanding of what is going
> on. The correct fix, whatever it is, would be determined by that
> understanding.
>
> In general, responding with a 403 does not invalidate the client
> connection, so it does not have to be closed. Said that, I am sure there
> are use cases where such closure would be a good idea. I would not be
> surprised if Squid closes the connection after denying regular requests.
>
>
> > Could you please give me some advice on a better/proper fix, if close()
> > at "abandoning" time is wrong?
>
> Unfortunately, I cannot. Somebody needs to investigate the problem and
> identify the bug in ConnStateData::kick() logic (or elsewhere).
>
> Alex.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20201225/f56bb97f/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reqmod_connect.patch
Type: text/x-patch
Size: 1788 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20201225/f56bb97f/attachment.bin>