patch was modified for applying to squid-3.4.14 ------------------------------------------------------------ revno: 13991 revision-id: rousskov@measurement-factory.com-20160219231541-syrgnvl1av8bbn8d parent: rousskov@measurement-factory.com-20160218041533-8tmtd45c3nky2gyy committer: Alex Rousskov branch nick: 3.5 timestamp: Fri 2016-02-19 16:15:41 -0700 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: rousskov@measurement-factory.com-20160219231541-\ # syrgnvl1av8bbn8d # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 3a9c41e0584065e737250cf9f8eb9eea7a85e9ba # timestamp: 2016-02-19 23:50:57 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: rousskov@measurement-factory.com-20160218041533-\ # 8tmtd45c3nky2gyy # # Begin patch === modified file 'src/SquidString.h' --- a/src/SquidString.h +++ b/src/SquidString.h @@ -80,6 +80,13 @@ _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 @@ _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); === modified file 'src/StrList.cc' --- 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 */ === modified file 'src/String.cc' --- a/src/String.cc +++ b/src/String.cc @@ -42,7 +42,7 @@ String::setBuffer(char *aBuf, String::size_type aSize) { assert(undefined()); - assert(aSize < 65536); + assert(aSize <= SizeMax_); buf_ = aBuf; size_ = aSize; } @@ -171,7 +171,7 @@ } 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); === modified file 'src/Server.cc' --- a/src/Server.cc +++ b/src/Server.cc @@ -49,6 +49,7 @@ startedAdaptation(false), #endif receivedWholeRequestBody(false), + doneWithFwd(NULL), theVirginReply(NULL), theFinalReply(NULL) { @@ -74,8 +75,6 @@ HTTPMSGUNLOCK(theVirginReply); HTTPMSGUNLOCK(theFinalReply); - fwd = NULL; // refcounted - if (responseBodyBuffer != NULL) { delete responseBodyBuffer; responseBodyBuffer = NULL; @@ -93,6 +92,14 @@ cleanAdaptation(); #endif + if (!doneWithServer()) + closeServer(); + + if (!doneWithFwd) { + doneWithFwd = "swanSong()"; + fwd->handleUnregisteredServerEnd(); + } + BodyConsumer::swanSong(); #if USE_ADAPTATION Initiator::swanSong(); @@ -218,6 +225,7 @@ { debugs(11,5, HERE << "completing forwarding for " << fwd); assert(fwd != NULL); + doneWithFwd = "completeForwarding()"; fwd->complete(); } === modified file 'src/Server.h' --- a/src/Server.h +++ b/src/Server.h @@ -176,6 +176,10 @@ #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(); === modified file 'src/ftp.cc' --- a/src/ftp.cc +++ b/src/ftp.cc @@ -839,6 +839,7 @@ { debugs(9, 4, HERE); ctrl.clear(); + doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too mustStop("FtpStateData::ctrlClosed"); } === modified file 'src/http.cc' --- a/src/http.cc +++ b/src/http.cc @@ -152,6 +152,7 @@ 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"); } @@ -2407,21 +2409,11 @@ ServerStateData::sentRequestBody(io); } -// Quickly abort the transaction -// TODO: destruction should be sufficient as the destructor should cleanup, -// including canceling close handlers void HttpStateData::abortTransaction(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::abortTransaction"); + mustStop(reason); }