From: Alex Rousskov Date: Fri, 19 Feb 2016 23:15:41 +0000 (-0700) Subject: Throw instead of asserting on some String overflows. X-Git-Tag: SQUID_3_5_15~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=95f44edc564364fcf424ba1ce8b7efb88c690959;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. 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 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..ae69ff5a1b 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 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 fa9b37b2b7..9acca44615 100644 --- 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 88e59d5a64..e479fe38c6 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(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 c27743b75b..8181cd1007 100644 --- 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 30fb94a1c2..bf4a5add07 100644 --- 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 178b380821..eb148824e0 100644 --- a/src/http.cc +++ b/src/http.cc @@ -152,6 +152,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"); } @@ -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); }