parent
80d8cd46d8
commit
552c48d5d7
@ -0,0 +1,117 @@
|
|||||||
|
From 4d6dd3ddba5e850a42c86d8233735165a371c31c Mon Sep 17 00:00:00 2001
|
||||||
|
From: Alex Rousskov <rousskov@measurement-factory.com>
|
||||||
|
Date: Sun, 1 Sep 2024 00:39:34 +0000
|
||||||
|
Subject: [PATCH] Bug 5405: Large uploads fill request buffer and die (#1887)
|
||||||
|
|
||||||
|
maybeMakeSpaceAvailable: request buffer full
|
||||||
|
ReadNow: ... size 0, retval 0, errno 0
|
||||||
|
terminateAll: 1/1 after ERR_CLIENT_GONE/WITH_CLIENT
|
||||||
|
%Ss=TCP_MISS_ABORTED
|
||||||
|
|
||||||
|
This bug is triggered by a combination of the following two conditions:
|
||||||
|
|
||||||
|
* HTTP client upload fills Squid request buffer faster than it is
|
||||||
|
drained by an origin server, cache_peer, or REQMOD service. The buffer
|
||||||
|
accumulates 576 KB (default 512 KB client_request_buffer_max_size + 64
|
||||||
|
KB internal "pipe" buffer).
|
||||||
|
|
||||||
|
* The affected server or service consumes a few bytes after the critical
|
||||||
|
accumulation is reached. In other words, the bug cannot be triggered
|
||||||
|
if nothing is consumed after the first condition above is met.
|
||||||
|
|
||||||
|
Comm::ReadNow() must not be called with a full buffer: Related
|
||||||
|
FD_READ_METHOD() code cannot distinguish "received EOF" from "had no
|
||||||
|
buffer space" outcomes. Server::readSomeData() tried to prevent such
|
||||||
|
calls, but the corresponding check had two problems:
|
||||||
|
|
||||||
|
* The check had an unsigned integer underflow bug[^1] that made it
|
||||||
|
ineffective when inBuf length exceeded Config.maxRequestBufferSize.
|
||||||
|
That length could exceed the limit due to reconfiguration and when
|
||||||
|
inBuf space size first grew outside of maybeMakeSpaceAvailable()
|
||||||
|
protections (e.g., during an inBuf.c_str() call) and then got filled
|
||||||
|
with newly read data. That growth started happening after 2020 commit
|
||||||
|
1dfbca06 optimized SBuf::cow() to merge leading and trailing space.
|
||||||
|
Prior to that commit, Bug 5405 could probably only affect Squid
|
||||||
|
reconfigurations that lower client_request_buffer_max_size.
|
||||||
|
|
||||||
|
* The check was separated from the ReadNow() call it was meant to
|
||||||
|
protect. While ConnStateData was waiting for the socket to become
|
||||||
|
ready for reading, various asynchronous events could alter inBuf or
|
||||||
|
Config.maxRequestBufferSize.
|
||||||
|
|
||||||
|
This change fixes both problems.
|
||||||
|
|
||||||
|
This change also fixes Squid Bug 5214.
|
||||||
|
|
||||||
|
[^1]: That underflow bug was probably introduced in 2015 commit 4d1376d7
|
||||||
|
while trying to emulate the original "do not read less than two bytes"
|
||||||
|
ConnStateData::In::maybeMakeSpaceAvailable() condition. That condition
|
||||||
|
itself looks like a leftover from manual zero-terminated input buffer
|
||||||
|
days that ended with 2014 commit e7287625. It is now removed.
|
||||||
|
---
|
||||||
|
|
||||||
|
diff --git a/src/servers/Server.cc b/src/servers/Server.cc
|
||||||
|
index 70fd10b..dd20619 100644
|
||||||
|
--- a/src/servers/Server.cc
|
||||||
|
+++ b/src/servers/Server.cc
|
||||||
|
@@ -83,16 +83,25 @@ Server::maybeMakeSpaceAvailable()
|
||||||
|
debugs(33, 4, "request buffer full: client_request_buffer_max_size=" << Config.maxRequestBufferSize);
|
||||||
|
}
|
||||||
|
|
||||||
|
+bool
|
||||||
|
+Server::mayBufferMoreRequestBytes() const
|
||||||
|
+{
|
||||||
|
+ // TODO: Account for bodyPipe buffering as well.
|
||||||
|
+ if (inBuf.length() >= Config.maxRequestBufferSize) {
|
||||||
|
+ debugs(33, 4, "no: " << inBuf.length() << '-' << Config.maxRequestBufferSize << '=' << (inBuf.length() - Config.maxRequestBufferSize));
|
||||||
|
+ return false;
|
||||||
|
+ }
|
||||||
|
+ debugs(33, 7, "yes: " << Config.maxRequestBufferSize << '-' << inBuf.length() << '=' << (Config.maxRequestBufferSize - inBuf.length()));
|
||||||
|
+ return true;
|
||||||
|
+}
|
||||||
|
+
|
||||||
|
void
|
||||||
|
Server::readSomeData()
|
||||||
|
{
|
||||||
|
if (reading())
|
||||||
|
return;
|
||||||
|
|
||||||
|
- debugs(33, 4, clientConnection << ": reading request...");
|
||||||
|
-
|
||||||
|
- // we can only read if there is more than 1 byte of space free
|
||||||
|
- if (Config.maxRequestBufferSize - inBuf.length() < 2)
|
||||||
|
+ if (!mayBufferMoreRequestBytes())
|
||||||
|
return;
|
||||||
|
|
||||||
|
typedef CommCbMemFunT<Server, CommIoCbParams> Dialer;
|
||||||
|
@@ -123,7 +132,16 @@ Server::doClientRead(const CommIoCbParams &io)
|
||||||
|
* Plus, it breaks our lame *HalfClosed() detection
|
||||||
|
*/
|
||||||
|
|
||||||
|
+ // mayBufferMoreRequestBytes() was true during readSomeData(), but variables
|
||||||
|
+ // like Config.maxRequestBufferSize may have changed since that check
|
||||||
|
+ if (!mayBufferMoreRequestBytes()) {
|
||||||
|
+ // XXX: If we avoid Comm::ReadNow(), we should not Comm::Read() again
|
||||||
|
+ // when the wait is over; resume these doClientRead() checks instead.
|
||||||
|
+ return; // wait for noteMoreBodySpaceAvailable() or a similar inBuf draining event
|
||||||
|
+ }
|
||||||
|
maybeMakeSpaceAvailable();
|
||||||
|
+ Assure(inBuf.spaceSize());
|
||||||
|
+
|
||||||
|
CommIoCbParams rd(this); // will be expanded with ReadNow results
|
||||||
|
rd.conn = io.conn;
|
||||||
|
switch (Comm::ReadNow(rd, inBuf)) {
|
||||||
|
diff --git a/src/servers/Server.h b/src/servers/Server.h
|
||||||
|
index ef105f5..6e549b3 100644
|
||||||
|
--- a/src/servers/Server.h
|
||||||
|
+++ b/src/servers/Server.h
|
||||||
|
@@ -119,6 +119,9 @@ protected:
|
||||||
|
/// abort any pending transactions and prevent new ones (by closing)
|
||||||
|
virtual void terminateAll(const Error &, const LogTagsErrors &) = 0;
|
||||||
|
|
||||||
|
+ /// whether client_request_buffer_max_size allows inBuf.length() increase
|
||||||
|
+ bool mayBufferMoreRequestBytes() const;
|
||||||
|
+
|
||||||
|
void doClientRead(const CommIoCbParams &io);
|
||||||
|
void clientWriteDone(const CommIoCbParams &io);
|
||||||
|
|
Loading…
Reference in new issue