Author: Graham Keeling <graham@equiinet.com>

Bug 3113: Squid can eat far too much memory when uploading files

Problem description:
  Uploading a large file to a web site on the internet, squid's client
input buffer will increase far faster than it can be emptied to
the target website, and the machine will swiftly run out of memory.

This patch adds the client_request_buffer_max_size configuration
parameter  which specifies the maximum buffer size of a client request.

diff --git a/doc/release-notes/release-3.1.sgml b/doc/release-notes/release-3.1.sgml
index 9629f5ae66f515c17cf324e4a6d39cf884ac0271..518f7ad4f420eeec34a01e2be857963485a9b6a0 100644 (file)
--- a/doc/release-notes/release-3.1.sgml
+++ b/doc/release-notes/release-3.1.sgml
@@ -563,6 +563,10 @@ This section gives a thorough account of those changes in three categories:
        direct client address in delay pools.
        </verb>
 
+       <tag>client_request_buffer_max_size</tag>
+       <p>New directive added with squid-3.1.10 to set limits on the amount of buffer space allocated
+       for receiving upload and request data from clients.
+
         <tag>dns_v4_fallback</tag>
         <p>New option to prevent Squid from always looking up IPv4 regardless of whether IPv6 addresses are found.
            Squid will follow a policy of prefering IPv6 links, keeping the IPv4 only as a safety net behind IPv6.

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 203d621abdd7aada55b98bf9b3afa82049f2c2f5..8a795e59c449a082592c1ce1cf3ef23b12fe049b 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -736,6 +736,14 @@ configDoConfigure(void)
     }
 
 #endif
+
+    // prevent infinite fetch loops in the request parser
+    // due to buffer full but not enough data recived to finish parse
+    if (Config.maxRequestBufferSize <= Config.maxRequestHeaderSize) {
+        fatalf("Client request buffer of %u bytes cannot hold a request with %u bytes of headers." \
+               " Change client_request_buffer_max or request_header_max_size limits.",
+               (uint32_t)Config.maxRequestBufferSize, (uint32_t)Config.maxRequestHeaderSize);
+    }
 }
 
 /* Parse a time specification from the config file.  Store the

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 55b82219b5754b47164d9dba4f76faa4f651d5e8..6a2d76c846181f50663d7f0898ff72dc4302a546 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -3541,6 +3541,17 @@ DOC_START
        be no limit imposed.
 DOC_END
 
+NAME: client_request_buffer_max_size
+COMMENT: (bytes)
+TYPE: b_size_t
+DEFAULT: 512 KB
+LOC: Config.maxRequestBufferSize
+DOC_START
+       This specifies the maximum buffer size of a client request.
+       It prevents squid eating too much memory when somebody uploads
+       a large file.
+DOC_END
+
 NAME: chunked_request_body_max_size
 COMMENT: (bytes)
 TYPE: b_int64_t

diff --git a/src/client_side.cc b/src/client_side.cc
index 8f28eb0879967c94fcd0fe4584233be2d0ddb0b2..a7d6c61f6b6293fd844f3f2e28c7e4624d48cede 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -207,7 +207,8 @@ ConnStateData::readSomeData()
 
     debugs(33, 4, "clientReadSomeData: FD " << fd << ": reading request...");
 
-    makeSpaceAvailable();
+    if (!maybeMakeSpaceAvailable())
+        return;
 
     typedef CommCbMemFunT<ConnStateData, CommIoCbParams> Dialer;
     reader = JobCallback(33, 5,
@@ -2153,13 +2154,22 @@ ConnStateData::getAvailableBufferLength() const
     return result;
 }
 
-void
-ConnStateData::makeSpaceAvailable()
+bool
+ConnStateData::maybeMakeSpaceAvailable()
 {
     if (getAvailableBufferLength() < 2) {
-        in.buf = (char *)memReallocBuf(in.buf, in.allocatedSize * 2, &in.allocatedSize);
+        size_t newSize;
+        if (in.allocatedSize >= Config.maxRequestBufferSize) {
+            debugs(33, 4, "request buffer full: client_request_buffer_max_size=" << Config.maxRequestBufferSize);
+            return false;
+        }
+        if ((newSize=in.allocatedSize * 2) > Config.maxRequestBufferSize) {
+            newSize=Config.maxRequestBufferSize;
+        }
+        in.buf = (char *)memReallocBuf(in.buf, newSize, &in.allocatedSize);
         debugs(33, 2, "growing request buffer: notYetUsed=" << in.notYetUsed << " size=" << in.allocatedSize);
     }
+    return true;
 }
 
 void
@@ -2873,7 +2883,10 @@ ConnStateData::handleRequestBodyData()
 void
 ConnStateData::noteMoreBodySpaceAvailable(BodyPipe::Pointer )
 {
-    handleRequestBodyData();
+    if (!handleRequestBodyData())
+        return;
+
+    readSomeData();
 }
 
 void

diff --git a/src/client_side.h b/src/client_side.h
index 0ec5ce5d5f158166d039f3f5476644154b41a5c0..97ce655a4df37075e333ec822dbaa3a5819a0e4b 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -139,7 +139,7 @@ public:
     bool areAllContextsForThisConnection() const;
     void freeAllContexts();
     void readNextRequest();
-    void makeSpaceAvailable();
+    bool maybeMakeSpaceAvailable();
     ClientSocketContext::Pointer getCurrentContext() const;
     void addContextToQueue(ClientSocketContext * context);
     int getConcurrentRequestCount() const;

diff --git a/src/structs.h b/src/structs.h
index 8aaeabea821afa051c1e66867efc24ff0557939b..42d2e82e2c8d1c3eef3d72bb55fb70dcd58a0674 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -186,6 +186,7 @@ struct SquidConfig {
     size_t maxRequestHeaderSize;
     int64_t maxRequestBodySize;
     int64_t maxChunkedRequestBodySize;
+    size_t maxRequestBufferSize;
     size_t maxReplyHeaderSize;
     acl_size_t *ReplyBodySize;