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. See trunk r8266 for a
similar fix with a detailed discussion. Here, I added doneWithFwd
instead of setting fwd to NULL because we dereference fwd (and store
pointers to things stored in fwd!) in many places. I think it is too
risky to just clear refcounted FwdState pointer (except in the
destructor where doing so is pointless).

Using doneWithFwd correctly is difficult because there are many ways we
can be "done" with FwdState, including:

    * calling fwd->complete(),
    * calling fwd->handleUnregisteredServerEnd(), and
    * closing the connection that FwdState monitors for closures.

The latter is especially tricky case because the closing is initiated in
many places, the process is asynchronous, and not all control
connections are monitored by FwdState.

For example, the updated control connection closure handler assumes that
it is being used for either external closures or internal closures
incorrectly used instead of mustStop()/abortAll(). In both cases, either
FwdState is still monitoring the connection (OK) or we forgot to call
one of its "done" methods listed above before closing. The latter would
be a bug, but I did not find any signs of it and fixing it would be
outside this change scope anyway.

Also unified String size limit checks [that I could find].

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..9a508cdb817b727c4d22fb2f8efbc4943418c819 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 auto 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 6fc6340c7b542587b1fb9ea67f1640fb52dc1b85..6c6ef53d5ada4bcca6543e795ac9fd139626bf17 100644 (file)
--- a/src/String.cc
+++ b/src/String.cc
@@ -41,7 +41,7 @@ void
 String::setBuffer(char *aBuf, String::size_type aSize)
 {
     assert(undefined());
-    assert(aSize < 65536);
+    assert(aSize <= SizeMax_);
     buf_ = aBuf;
     size_ = aSize;
 }
@@ -170,7 +170,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 f582d95d468178bfde82a85b87fdb77077e2b768..b346003b4141898472647c89d5931e1e34c6153a 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(nullptr),
     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 246647eb858fde37a764b853757fd40a67549bda..086b612bc5008fd2c7a5a3a630d9a4e6cf46a971 100644 (file)
--- a/src/clients/Client.h
+++ b/src/clients/Client.h
@@ -179,6 +179,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 659407ca5abb3b5bc3ddbb4da03e84664292d28f..0c4a2503d08f70b03cc235325d04882acd9c21bd 100644 (file)
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -841,6 +841,7 @@ Ftp::Client::ctrlClosed(const CommCloseCbParams &)
 {
     debugs(9, 4, status());
     ctrl.clear();
+    doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too
     mustStop("Ftp::Client::ctrlClosed");
 }
 
@@ -993,24 +994,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 e039cfbd46b91fcd344d2613178d38bc3e3cc23d..8bc14a3f46e648ca4409246bd962e484871c094a 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -153,6 +153,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");
 }
 
@@ -1825,7 +1826,8 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
 
         String strFwd = hdr_in->getList(Http::HdrType::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?
@@ -2462,21 +2464,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);
 }