From e55459fccbfdfeabf31f7906fc2c12f93f3df9c2 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Thu, 23 Sep 2010 07:47:21 -0600 Subject: [PATCH] Author: Alex Rousskov Bug 2311: 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 98b979e24c..799f81b499 100644 --- a/src/BodyPipe.cc +++ b/src/BodyPipe.cc @@ -337,7 +337,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 @@ -454,8 +454,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