Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit Server::inBuf growth #1898

Closed
wants to merge 2 commits into from
Closed

Limit Server::inBuf growth #1898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4348e0
Limit Server::inBuf growth
eduard-bagdasaryan Aug 30, 2024
d083875
Added an assurance before subtracting two size_t values
eduard-bagdasaryan Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/adaptation/icap/Xaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ void Adaptation::Icap::Xaction::noteCommRead(const CommIoCbParams &io)

CommIoCbParams rd(this); // will be expanded with ReadNow results
rd.conn = io.conn;
rd.size = SQUID_TCP_SO_RCVBUF - readBuf.length();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


switch (Comm::ReadNow(rd, readBuf)) {
case Comm::INPROGRESS:
Expand Down
3 changes: 3 additions & 0 deletions src/servers/Server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ Server::doClientRead(const CommIoCbParams &io)

CommIoCbParams rd(this); // will be expanded with ReadNow results
rd.conn = io.conn;
Assure(Config.maxRequestBufferSize > inBuf.length());
rd.size = Config.maxRequestBufferSize - inBuf.length();
Comment on lines +149 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


switch (Comm::ReadNow(rd, inBuf)) {
case Comm::INPROGRESS:

Expand Down