Limit Server::inBuf growth

[eduard-bagdasaryan]

After a ReadNow() call, the buffer length must not exceed accumulation
limits (e.g., client_request_buffer_max_size). SBuf::reserve() alone
cannot reliably enforce those limits because it does not decrease SBuf
space; various SBuf manipulations may lead to excessive SBuf space. When
filled by ReadNow(), that space exceeds the limit.

This change uses documented CommIoCbParams::size trick to limit how much
Comm::ReadNow() may read, obeying SQUID_TCP_SO_RCVBUF (server-to-Squid)
and client_request_buffer_max_size (client-to-Squid) accumulation limit.

[yadij]

Please be aware that maybeMakeSpaceAvailable() already restricts capacity to Config.maxRequestBufferSize.

FYI "0" is a special case. So, when this buffer contains maxRequestBufferSize bytes, the behaviour you are trying to avoid will occur despite this change.

[rousskov]

Please be aware that maybeMakeSpaceAvailable() already restricts capacity to Config.maxRequestBufferSize.

maybeMakeSpaceAvailable() does not restrict capacity (enough): The method may increase capacity up to the specified maximum (if needed), but it does not shrink already excessive capacity.

when this buffer contains maxRequestBufferSize bytes, the behaviour you are trying to avoid will occur despite this change.

Existing mayBufferMoreRequestBytes() check executed a few lines earlier makes the precondition in the above statement false -- this buffer cannot contain maxRequestBufferSize bytes at this point. That check was added in recent master commit 4d6dd3d to fix another bug. That is why this PR can assert that maxRequestBufferSize exceeds inBuf.length() and, hence, this rd.size math works without underflows.

[yadij]

FYI "0" is a special case. So, when this buffer contains SQUID_TCP_SO_RCVBUF bytes, the behaviour you are trying to avoid may occur despite this change.

I believe the fix you actually want is to use SBuf::reserve to set the buffer limits properly (fixing the above TODO reservation and related lines). Then enforce the resulting space available. Like so:

    rd.size = readBuf.reserve({ .maxCapacity=SQUID_TCP_SO_RCVBUF });
    Assure(rd.size <= SQUID_TCP_SO_RCVBUF);

[rousskov]

when this buffer contains SQUID_TCP_SO_RCVBUF bytes, the behaviour you are trying to avoid may occur despite this change.

Existing Must(readBuf.length() < SQUID_TCP_SO_RCVBUF) code executed a few lines earlier makes the precondition in the above statement false -- the buffer cannot contain SQUID_TCP_SO_RCVBUF bytes at this point.

I believe the fix you actually want is to use SBuf::reserve to set the buffer limits properly

We have considered that approach and ruled it out: SBuf::reserve() is not the right solution for the problem targeted by this PR because SBuf::reserve() must not shrink existing SBuf capacity. In modern C++, both std::vector::reserve() and std::string::reserve() do not shrink the underlying storage. C++ folks even explicitly prohibited such shrinking for strings in C++20!

We could consider adding a dedicated buffer shrinking method, but since we already have a documented API for avoiding excessive reads (without buffer shrinking), and lack performance data to justify shrinking overheads, we should use that rd.size API to fix the problem.

[rousskov]

@yadij, thank you for approving this PR. I have corrected a couple of misunderstandings in my comments and cleared this PR for merging.

[rousskov]

when this buffer contains SQUID_TCP_SO_RCVBUF bytes, the behaviour you are trying to avoid may occur despite this change.

Existing Must(readBuf.length() < SQUID_TCP_SO_RCVBUF) code executed a few lines earlier makes the precondition in the above statement false -- the buffer cannot contain SQUID_TCP_SO_RCVBUF bytes at this point.

I believe the fix you actually want is to use SBuf::reserve to set the buffer limits properly

We have considered that approach and ruled it out: SBuf::reserve() is not the right solution for the problem targeted by this PR because SBuf::reserve() must not shrink existing SBuf capacity. In modern C++, both std::vector::reserve() and std::string::reserve() do not shrink the underlying storage. C++ folks even explicitly prohibited such shrinking for strings in C++20!

We could consider adding a dedicated buffer shrinking method, but since we already have a documented API for avoiding excessive reads (without buffer shrinking), and lack performance data to justify shrinking overheads, we should use that rd.size API to fix the problem.

[rousskov]

Please be aware that maybeMakeSpaceAvailable() already restricts capacity to Config.maxRequestBufferSize.

maybeMakeSpaceAvailable() does not restrict capacity (enough): The method may increase capacity up to the specified maximum (if needed), but it does not shrink already excessive capacity.

when this buffer contains maxRequestBufferSize bytes, the behaviour you are trying to avoid will occur despite this change.

Existing mayBufferMoreRequestBytes() check executed a few lines earlier makes the precondition in the above statement false -- this buffer cannot contain maxRequestBufferSize bytes at this point. That check was added in recent master commit 4d6dd3d to fix another bug. That is why this PR can assert that maxRequestBufferSize exceeds inBuf.length() and, hence, this rd.size math works without underflows.