From: Alex Rousskov Date: Fri, 19 Feb 2016 21:23:08 +0000 (-0700) Subject: Throw instead of asserting on some String overflows. X-Git-Tag: SQUID_4_0_7~7 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=70df76e30151bb5fd2070a275e2489ee6bdaaea9;p=thirdparty%2Fsquid.git 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 4e7503fb31..c452308721 100644 --- 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 a0325eec03..9a508cdb81 100644 --- 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 6fc6340c7b..6c6ef53d5a 100644 --- 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 f582d95d46..b346003b41 100644 --- 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 246647eb85..086b612bc5 100644 --- 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 659407ca5a..0c4a2503d0 100644 --- 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 e039cfbd46..8bc14a3f46 100644 --- a/src/http.cc +++ b/src/http.cc @@ -153,6 +153,7 @@ void HttpStateData::httpStateConnClosed(const CommCloseCbParams ¶ms) { 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); }