Throw instead of asserting on some String overflows.

Note that Client-caught exceptions result in HTTP 500 (Internal Server
Error) responses with X-Squid-Error set to "ERR_CANNOT_FORWARD 0".

Also avoid stuck Client jobs on exceptions.

Also unified String size limit checks.

Essentially trunk r14552, which has a detailed commit message.

diff --git a/src/SquidString.h b/src/SquidString.h
index 4e7503fb31f2b869ac084ecac4312ec2053a6a45..c4523087213c23212898795c2bb2b0a3935ab54a 100644 (file)
--- a/src/SquidString.h
+++ b/src/SquidString.h
@@ -80,6 +80,13 @@ public:
     _SQUID_INLINE_ int caseCmp(char const *, size_type count) const;
     _SQUID_INLINE_ int caseCmp(String const &) const;
 
+    /// Whether creating a totalLen-character string is safe (i.e., unlikely to assert).
+    /// Optional extras can be used for overflow-safe length addition.
+    /// Implementation has to add 1 because many String allocation methods do.
+    static bool CanGrowTo(size_type totalLen, const size_type extras = 0) { return SafeAdd(totalLen, extras) && SafeAdd(totalLen, 1); }
+    /// whether appending growthLen characters is safe (i.e., unlikely to assert)
+    bool canGrowBy(const size_type growthLen) const { return CanGrowTo(size(), growthLen); }
+
     String substr(size_type from, size_type to) const;
 
     _SQUID_INLINE_ void cut(size_type newLength);
@@ -95,10 +102,14 @@ private:
     _SQUID_INLINE_ bool nilCmp(bool, bool, int &) const;
 
     /* never reference these directly! */
-    size_type size_; /* buffer size; 64K limit */
+    size_type size_; /* buffer size; limited by SizeMax_ */
 
     size_type len_;  /* current length  */
 
+    static const size_type SizeMax_ = 65535; ///< 64K limit protects some fixed-size buffers
+    /// returns true after increasing the first argument by extra if the sum does not exceed SizeMax_
+    static bool SafeAdd(size_type &base, size_type extra) { if (extra <= SizeMax_ && base <= SizeMax_ - extra) { base += extra; return true; } return false; }
+
     char *buf_;
 
     _SQUID_INLINE_ void set(char const *loc, char const ch);

diff --git a/src/StrList.cc b/src/StrList.cc
index a0325eec0364a91bfbc2e52d394ec38d9eb3b22a..ae69ff5a1bf38d0170b834a19ad16a9296a8875e 100644 (file)
--- a/src/StrList.cc
+++ b/src/StrList.cc
@@ -11,20 +11,24 @@
 #include "squid.h"
 #include "SquidString.h"
 #include "StrList.h"
+#include "base/TextException.h" 
 
 /** appends an item to the list */
 void
 strListAdd(String * str, const char *item, char del)
 {
     assert(str && item);
+    const String::size_type itemSize = strlen(item);
     if (str->size()) {
         char buf[3];
         buf[0] = del;
         buf[1] = ' ';
         buf[2] = '\0';
+        Must(str->canGrowBy(2));
         str->append(buf, 2);
     }
-    str->append(item, strlen(item));
+    Must(str->canGrowBy(itemSize));
+    str->append(item, itemSize);
 }
 
 /** returns true iff "m" is a member of the list */

diff --git a/src/String.cc b/src/String.cc
index fa9b37b2b7c37fcefd4b0a73908e4765e3e6f2bc..9acca44615e6a13e89980e150efb5cd730af2b12 100644 (file)
--- a/src/String.cc
+++ b/src/String.cc
@@ -42,7 +42,7 @@ void
 String::setBuffer(char *aBuf, String::size_type aSize)
 {
     assert(undefined());
-    assert(aSize < 65536);
+    assert(aSize <= SizeMax_);
     buf_ = aBuf;
     size_ = aSize;
 }
@@ -171,7 +171,7 @@ String::append( char const *str, int len)
     } else {
         // Create a temporary string and absorb it later.
         String snew;
-        assert(len_ + len < 65536); // otherwise snew.len_ overflows below
+        assert(canGrowBy(len)); // otherwise snew.len_ may overflow below
         snew.len_ = len_ + len;
         snew.allocBuffer(snew.len_ + 1);
 

diff --git a/src/clients/Client.cc b/src/clients/Client.cc
index 88e59d5a64d39cfe6c2c6079b667b67e096a59eb..e479fe38c6e9a5460a7318837e074214bf5da92f 100644 (file)
--- a/src/clients/Client.cc
+++ b/src/clients/Client.cc
@@ -49,6 +49,7 @@ Client::Client(FwdState *theFwdState): AsyncJob("Client"),
     startedAdaptation(false),
 #endif
     receivedWholeRequestBody(false),
+    doneWithFwd(NULL),
     theVirginReply(NULL),
     theFinalReply(NULL)
 {
@@ -74,8 +75,6 @@ Client::~Client()
     HTTPMSGUNLOCK(theVirginReply);
     HTTPMSGUNLOCK(theFinalReply);
 
-    fwd = NULL; // refcounted
-
     if (responseBodyBuffer != NULL) {
         delete responseBodyBuffer;
         responseBodyBuffer = NULL;
@@ -93,6 +92,14 @@ Client::swanSong()
     cleanAdaptation();
 #endif
 
+    if (!doneWithServer())
+        closeServer();
+
+    if (!doneWithFwd) {
+        doneWithFwd = "swanSong()";
+        fwd->handleUnregisteredServerEnd();
+    }
+
     BodyConsumer::swanSong();
 #if USE_ADAPTATION
     Initiator::swanSong();
@@ -218,6 +225,7 @@ Client::completeForwarding()
 {
     debugs(11,5, HERE << "completing forwarding for "  << fwd);
     assert(fwd != NULL);
+    doneWithFwd = "completeForwarding()";
     fwd->complete();
 }
 

diff --git a/src/clients/Client.h b/src/clients/Client.h
index c27743b75be837f99086ef8bd5335b65160b4d88..8181cd10079a6ce69bd3602b4b6cecd84c2bfc64 100644 (file)
--- a/src/clients/Client.h
+++ b/src/clients/Client.h
@@ -176,6 +176,10 @@ protected:
 #endif
     bool receivedWholeRequestBody; ///< handleRequestBodyProductionEnded called
 
+    /// whether we should not be talking to FwdState; XXX: clear fwd instead
+    /// points to a string literal which is used only for debugging
+    const char *doneWithFwd;
+
 private:
     void sendBodyIsTooLargeError();
     void maybePurgeOthers();

diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index 30fb94a1c29bab3ac8dc542d9ae7b2fdf3a6f023..bf4a5add070fc2a8a5df70c587b8b4657edf6d2a 100644 (file)
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -839,6 +839,7 @@ Ftp::Client::ctrlClosed(const CommCloseCbParams &io)
 {
     debugs(9, 4, status());
     ctrl.clear();
+    doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too
     mustStop("Ftp::Client::ctrlClosed");
 }
 
@@ -991,24 +992,12 @@ Ftp::Client::dataComplete()
     scheduleReadControlReply(1);
 }
 
-/**
- * Quickly abort the transaction
- *
- \todo destruction should be sufficient as the destructor should cleanup,
- * including canceling close handlers
- */
 void
 Ftp::Client::abortAll(const char *reason)
 {
     debugs(9, 3, "aborting transaction for " << reason <<
            "; FD " << (ctrl.conn!=NULL?ctrl.conn->fd:-1) << ", Data FD " << (data.conn!=NULL?data.conn->fd:-1) << ", this " << this);
-    if (Comm::IsConnOpen(ctrl.conn)) {
-        ctrl.conn->close();
-        return;
-    }
-
-    fwd->handleUnregisteredServerEnd();
-    mustStop("Ftp::Client::abortTransaction");
+    mustStop(reason);
 }
 
 /**

diff --git a/src/http.cc b/src/http.cc
index 178b3808216bfd5ca03864d6d1c84041b9dd1801..eb148824e00308c08cdbd9033564c45994602e0d 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -152,6 +152,7 @@ void
 HttpStateData::httpStateConnClosed(const CommCloseCbParams &params)
 {
     debugs(11, 5, "httpStateFree: FD " << params.fd << ", httpState=" << params.data);
+    doneWithFwd = "httpStateConnClosed()"; // assume FwdState is monitoring too
     mustStop("HttpStateData::httpStateConnClosed");
 }
 
@@ -1756,7 +1757,8 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
 
         String strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR);
 
-        if (strFwd.size() > 65536/2) {
+        // if we cannot double strFwd size, then it grew past 50% of the limit
+        if (!strFwd.canGrowBy(strFwd.size())) {
             // There is probably a forwarding loop with Via detection disabled.
             // If we do nothing, String will assert on overflow soon.
             // TODO: Terminate all transactions with huge XFF?
@@ -2407,21 +2409,11 @@ HttpStateData::sentRequestBody(const CommIoCbParams &io)
     Client::sentRequestBody(io);
 }
 
-// Quickly abort the transaction
-// TODO: destruction should be sufficient as the destructor should cleanup,
-// including canceling close handlers
 void
 HttpStateData::abortAll(const char *reason)
 {
     debugs(11,5, HERE << "aborting transaction for " << reason <<
            "; " << serverConnection << ", this " << this);
-
-    if (Comm::IsConnOpen(serverConnection)) {
-        serverConnection->close();
-        return;
-    }
-
-    fwd->handleUnregisteredServerEnd();
-    mustStop("HttpStateData::abortAll");
+    mustStop(reason);
 }