From 87728a5b09981273234397c994418c6cfa286fa1 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sun, 5 Sep 2010 17:34:08 -0600 Subject: [PATCH] Bug #2311 fix: crashes with ICAP RESPMOD for HTTP body size greater than 100kb BodyPipe::undoCheckOut() must not assert that undo is possible because it is not (currently) possible if the pipe buffer was modified. BodyPipe::undoCheckOut() must not throw if undo is not possible because it is often called when there is already an exception thrown and because it is called from the BodyPipeCheckout destructor and destructors should not throw (this case is an illustration for one reason why they should not). Currently, we only use an implicit undo, and only when an exception is being thrown while the buffer is checked out. Currently, the code that does checkout is probably safe because it should terminate the transaction if a parser throws. However, this is not 100% guaranteed, and the situation may change without us noticing. TODO: consider implementing the long-term solution discussed at http://www.mail-archive.com/squid-dev@squid-cache.org/msg07910.html COW-buffers may help here as well. --- src/BodyPipe.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/BodyPipe.cc b/src/BodyPipe.cc index 4b8b6bd261..42c0f9e2c7 100644 --- a/src/BodyPipe.cc +++ b/src/BodyPipe.cc @@ -346,7 +346,7 @@ BodyPipe::undoCheckOut(Checkout &checkout) // raw buffers should always check them in (possibly unchanged) // instead of relying on the automated undo mechanism of Checkout. // The code can always use a temporary buffer to accomplish that. - assert(checkout.checkedOutSize == currentSize); + Must(checkout.checkedOutSize == currentSize); } // TODO: Optimize: inform consumer/producer about more data/space only if @@ -463,8 +463,13 @@ BodyPipeCheckout::BodyPipeCheckout(BodyPipe &aPipe): pipe(aPipe), BodyPipeCheckout::~BodyPipeCheckout() { - if (!checkedIn) - pipe.undoCheckOut(*this); + if (!checkedIn) { + // Do not pipe.undoCheckOut(*this) because it asserts or throws + // TODO: consider implementing the long-term solution discussed at + // http://www.mail-archive.com/squid-dev@squid-cache.org/msg07910.html + debugs(91,2, HERE << "Warning: cannot undo BodyPipeCheckout"); + pipe.checkIn(*this); + } } void -- 2.47.2