--------------------- PatchSet 3835 Date: 2006/10/06 16:26:15 Author: rousskov Branch: squid3-icap Tag: (none) Log: - Rewrote communication between a server-side ICAPClient*mod* vector and its owner. When a server-side ICAPClient*mod* vector was notifying its owner of more adapted data, the owner could delete the vector (by calling icap->ownerAbort) if the store entry was not willing to accept the data. The same deletion could happen when a vector was notifying the owner of a successful termination. In all those cases, the vector did not expect to be deleted and could continue to do something, causing segmentation faults. Now, when more data is available, the vector calls its owner and checks the return value of the call. If it is false, the vector knows it has been deleted and quits. When vector terminates, it calls its owner and trusts the owner to always delete the vector. The "check return value and quit" design is not perfect, but we are paying the price for isolating the vectors from their owners while using direct calls between them (instead of MsgPipe or a similar less efficient indirect approach we use elsewhere). Members: src/Server.h:1.1.12.2->1.1.12.3 src/ftp.cc:1.29.2.7->1.29.2.8 src/http.cc:1.49.2.56->1.49.2.57 src/http.h:1.11.4.17->1.11.4.18 Index: squid3/src/Server.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/Server.h,v retrieving revision 1.1.12.2 retrieving revision 1.1.12.3 diff -u -r1.1.12.2 -r1.1.12.3 --- squid3/src/Server.h 29 Sep 2006 23:16:00 -0000 1.1.12.2 +++ squid3/src/Server.h 6 Oct 2006 16:26:15 -0000 1.1.12.3 @@ -1,6 +1,6 @@ /* - * $Id: Server.h,v 1.1.12.2 2006/09/29 23:16:00 dwsquid Exp $ + * $Id: Server.h,v 1.1.12.3 2006/10/06 16:26:15 rousskov Exp $ * * AUTHOR: Duane Wessels * @@ -59,10 +59,9 @@ virtual ~ServerStateData(); #if ICAP_CLIENT - - virtual void takeAdaptedHeaders(HttpReply *) = 0; - virtual void takeAdaptedBody(MemBuf *) = 0; - virtual void doneAdapting() = 0; + virtual bool takeAdaptedHeaders(HttpReply *) = 0; + virtual bool takeAdaptedBody(MemBuf *) = 0; + virtual void finishAdapting() = 0; virtual void abortAdapting() = 0; virtual void icapSpaceAvailable() = 0; virtual void icapAclCheckDone(ICAPServiceRep::Pointer) = 0; Index: squid3/src/ftp.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/ftp.cc,v retrieving revision 1.29.2.7 retrieving revision 1.29.2.8 diff -u -r1.29.2.7 -r1.29.2.8 --- squid3/src/ftp.cc 3 Oct 2006 00:18:58 -0000 1.29.2.7 +++ squid3/src/ftp.cc 6 Oct 2006 16:26:16 -0000 1.29.2.8 @@ -1,6 +1,6 @@ /* - * $Id: ftp.cc,v 1.29.2.7 2006/10/03 00:18:58 rousskov Exp $ + * $Id: ftp.cc,v 1.29.2.8 2006/10/06 16:26:16 rousskov Exp $ * * DEBUG: section 9 File Transfer Protocol (FTP) * AUTHOR: Harvest Derived @@ -223,12 +223,14 @@ public: void icapAclCheckDone(ICAPServiceRep::Pointer); - void takeAdaptedHeaders(HttpReply *); - void takeAdaptedBody(MemBuf *); - void doneAdapting(); + bool takeAdaptedHeaders(HttpReply *); + bool takeAdaptedBody(MemBuf *); + void finishAdapting(); void abortAdapting(); void icapSpaceAvailable(); bool icapAccessCheckPending; +private: + void endAdapting(); #endif }; @@ -3386,15 +3388,14 @@ maybeReadData(); } -void +bool FtpStateData::takeAdaptedHeaders(HttpReply *rep) { debug(11,5)("FtpStateData::takeAdaptedHeaders() called\n"); if (!entry->isAccepting()) { debug(11,5)("\toops, entry is not Accepting!\n"); - icap->ownerAbort(); - return; + return false; } assert (rep); @@ -3404,9 +3405,10 @@ reply = HTTPMSGLOCK(rep); debug(11,5)("FtpStateData::takeAdaptedHeaders() finished\n"); + return true; } -void +bool FtpStateData::takeAdaptedBody(MemBuf *buf) { debug(11,5)("FtpStateData::takeAdaptedBody() called\n"); @@ -3414,36 +3416,26 @@ if (!entry->isAccepting()) { debug(11,5)("\toops, entry is not Accepting!\n"); - icap->ownerAbort(); - return; + return false; } storeAppend(entry, buf->content(), buf->contentSize()); buf->consume(buf->contentSize()); // consume everything written + return true; } void -FtpStateData::doneAdapting() +FtpStateData::finishAdapting() { debug(11,5)("FtpStateData::doneAdapting() called\n"); if (!entry->isAccepting()) { debug(11,5)("\toops, entry is not Accepting!\n"); - icap->ownerAbort(); + abortAdapting(); } else { - transactionForwardComplete(); + transactionForwardComplete(); + endAdapting(); } - - /* - * ICAP is done, so we don't need this any more. - */ - delete icap; - icap = NULL; - - if (ctrl.fd >= 0) - comm_close(ctrl.fd); - else - delete this; } void @@ -3451,12 +3443,6 @@ { debug(11,5)("FtpStateData::abortAdapting() called\n"); - /* - * ICAP has given up, we're done with it too - */ - delete icap; - icap = NULL; - if (entry->isEmpty()) { ErrorState *err; err = errorCon(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR, request); @@ -3465,10 +3451,20 @@ fwd->dontRetry(true); } + endAdapting(); +} + +void +FtpStateData::endAdapting() +{ + delete icap; + icap = NULL; + if (ctrl.fd >= 0) comm_close(ctrl.fd); else delete this; } + #endif Index: squid3/src/http.cc =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/http.cc,v retrieving revision 1.49.2.56 retrieving revision 1.49.2.57 diff -u -r1.49.2.56 -r1.49.2.57 --- squid3/src/http.cc 3 Oct 2006 00:18:58 -0000 1.49.2.56 +++ squid3/src/http.cc 6 Oct 2006 16:26:16 -0000 1.49.2.57 @@ -1,6 +1,6 @@ /* - * $Id: http.cc,v 1.49.2.56 2006/10/03 00:18:58 rousskov Exp $ + * $Id: http.cc,v 1.49.2.57 2006/10/06 16:26:16 rousskov Exp $ * * DEBUG: section 11 Hypertext Transfer Protocol (HTTP) * AUTHOR: Harvest Derived @@ -2021,15 +2021,15 @@ maybeReadData(); } -void +bool HttpStateData::takeAdaptedHeaders(HttpReply *rep) { debug(11,5)("HttpStateData::takeAdaptedHeaders() called\n"); if (!entry->isAccepting()) { debug(11,5)("\toops, entry is not Accepting!\n"); - icap->ownerAbort(); - return; + abortAdapting(); + return false; } assert (rep); @@ -2041,9 +2041,10 @@ haveParsedReplyHeaders(); debug(11,5)("HttpStateData::takeAdaptedHeaders() finished\n"); + return true; } -void +bool HttpStateData::takeAdaptedBody(MemBuf *buf) { debug(11,5)("HttpStateData::takeAdaptedBody() called\n"); @@ -2052,50 +2053,38 @@ if (!entry->isAccepting()) { debug(11,5)("\toops, entry is not Accepting!\n"); - icap->ownerAbort(); - return; + abortAdapting(); + return false; } entry->write(StoreIOBuffer(buf, currentOffset)); // write everything currentOffset += buf->contentSize(); buf->consume(buf->contentSize()); // consume everything written + return true; } +// called when ICAP adaptation is about to finish successfully, destroys icap void -HttpStateData::doneAdapting() +HttpStateData::finishAdapting() { - debug(11,5)("HttpStateData::doneAdapting() called\n"); + debug(11,5)("HttpStateData::takeAdaptingEnd() called\n"); - if (!entry->isAccepting()) { + if (!entry->isAccepting()) { // XXX: do we need this check here? debug(11,5)("\toops, entry is not Accepting!\n"); - icap->ownerAbort(); + abortAdapting(); } else { fwd->complete(); + endAdapting(); } - - /* - * ICAP is done, so we don't need this any more. - */ - delete icap; - icap = NULL; - - if (fd >= 0) - comm_close(fd); - else - httpStateFree(fd, this); } +// called when there was an ICAP error, destroys icap +// may be called by ICAP code or ours void HttpStateData::abortAdapting() { debug(11,5)("HttpStateData::abortAdapting() called\n"); - /* - * ICAP has given up, we're done with it too - */ - delete icap; - icap = NULL; - if (entry->isEmpty()) { ErrorState *err; err = errorCon(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR, request); @@ -2105,11 +2094,22 @@ flags.do_next_read = 0; } - if (fd >= 0) { + endAdapting(); +} + +// internal helper to delete icap and close the HTTP connection +void +HttpStateData::endAdapting() +{ + debug(11,5)("HttpStateData::endAdapting() called, deleting %p\n", icap); + + delete icap; + icap = NULL; + + if (fd >= 0) comm_close(fd); - } else { - httpStateFree(-1, this); // deletes this - } + else + httpStateFree(fd, this); // deletes us } #endif Index: squid3/src/http.h =================================================================== RCS file: /cvsroot/squid-sf//squid3/src/http.h,v retrieving revision 1.11.4.17 retrieving revision 1.11.4.18 diff -u -r1.11.4.17 -r1.11.4.18 --- squid3/src/http.h 29 Sep 2006 23:27:13 -0000 1.11.4.17 +++ squid3/src/http.h 6 Oct 2006 16:26:48 -0000 1.11.4.18 @@ -1,6 +1,6 @@ /* - * $Id: http.h,v 1.11.4.17 2006/09/29 23:27:13 dwsquid Exp $ + * $Id: http.h,v 1.11.4.18 2006/10/06 16:26:48 rousskov Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -71,13 +71,13 @@ void readReply(size_t len, comm_err_t flag, int xerrno); void maybeReadData(); int cacheableReply(); -#if ICAP_CLIENT - void takeAdaptedHeaders(HttpReply *); - void takeAdaptedBody(MemBuf *); - void doneAdapting(); - void abortAdapting(); - void icapSpaceAvailable(); +#if ICAP_CLIENT + virtual bool takeAdaptedHeaders(HttpReply *); + virtual bool takeAdaptedBody(MemBuf *); + virtual void finishAdapting(); // deletes icap + virtual void abortAdapting(); // deletes icap + virtual void icapSpaceAvailable(); #endif peer *_peer; /* peer request made to */ @@ -134,6 +134,10 @@ http_state_flags flags); static bool decideIfWeDoRanges (HttpRequest * orig_request); +#if ICAP_CLIENT + void endAdapting(); +#endif + private: CBDATA_CLASS2(HttpStateData); };