From: robertc <> Date: Sat, 2 Sep 2006 12:49:48 +0000 (+0000) Subject: Fix bug 772, by reading aborted data from connections when there is a pending POST... X-Git-Tag: SQUID_3_0_PRE5~157 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0b86805b0c1b3f18d4286f53200b43238ad97e4a;p=thirdparty%2Fsquid.git Fix bug 772, by reading aborted data from connections when there is a pending POST on the connection. This is still lacking an automated test to prevent regressions occuring again. Additionally the BodyReader API can possibly be tightened up to make this easier to read and implement. --- diff --git a/src/BodyReader.cc b/src/BodyReader.cc index dca2095139..da85a56b44 100644 --- a/src/BodyReader.cc +++ b/src/BodyReader.cc @@ -62,7 +62,7 @@ BodyReader::read(CBCB *callback, void *cbdata) if (nread > 0) { _available -= nread; - _remaining -= nread; + reduce_remaining(nread); } else { debugs(32,3,HERE << this << " " << "Help, read_func() ret " << nread); } @@ -145,3 +145,10 @@ BodyReader::consume(size_t size) return true; } + +void +BodyReader::reduce_remaining(size_t size) +{ + assert(size <= _remaining); + _remaining -= size; +} diff --git a/src/BodyReader.h b/src/BodyReader.h index 0087587b70..1f1f978cd9 100644 --- a/src/BodyReader.h +++ b/src/BodyReader.h @@ -23,6 +23,11 @@ public: int bytes_read; + /* reduce the number of bytes that the BodyReader is looking for. + * Will trigger an assertion if it tries to reduce below zero + */ + void reduce_remaining(size_t size); + private: size_t _remaining; size_t _available; @@ -31,7 +36,7 @@ private: /* * These are for interacting with things that * "provide" body content. ie, ConnStateData and - * ICAPReqMod after adapation. + * ICAPReqMod after adapation. */ BodyReadFunc *read_func; BodyAbortFunc *abort_func; diff --git a/src/client_side.cc b/src/client_side.cc index 3b0292bb2c..56c03c4212 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side.cc,v 1.731 2006/08/02 21:46:22 hno Exp $ + * $Id: client_side.cc,v 1.732 2006/09/02 06:49:48 robertc Exp $ * * DEBUG: section 33 Client-side Routines * AUTHOR: Duane Wessels @@ -580,6 +580,22 @@ ConnStateData::areAllContextsForThisConnection() const return true; } +BodyReader * +ConnStateData::body_reader() +{ + return body_reader_.getRaw(); +} + +void +ConnStateData::body_reader(BodyReader::Pointer reader) +{ + body_reader_ = reader; + if (reader == NULL) + fd_note(fd, "Waiting for next request"); + else + fd_note(fd, "Reading request body"); +} + void ConnStateData::freeAllContexts() { @@ -639,7 +655,7 @@ ConnStateData::~ConnStateData() cbdataReferenceDone(port); - body_reader = NULL; // refcounted + body_reader(NULL); // refcounted } /* @@ -1560,13 +1576,19 @@ ClientSocketContext::initiateClose() * this may be an issue. */ conn->closing(true); + /* any unread body becomes abortedSize at this point. */ + conn->in.abortedSize = conn->bodySizeLeft(); /* * Trigger the BodyReader abort handler, if necessary, * by destroying it. It is a refcounted pointer, so * set it to NULL and let the destructor be called when * all references are gone. + * + * This seems to be flawed: theres no way this can trigger + * if conn->body_reader is not NULL. Perhaps it works for + * ICAP but not real requests ? */ - http->request->body_reader = NULL; // refcounted + http->request->body_reader = NULL; // refcounted return; } } @@ -2341,7 +2363,7 @@ clientProcessRequest(ConnStateData::Pointer &conn, ClientSocketContext *context, clientAbortBody, NULL, conn.getRaw()); - conn->body_reader = request->body_reader; + conn->body_reader(request->body_reader); request->body_reader->notify(conn->in.notYetUsed); if (request->body_reader->remaining()) @@ -2408,8 +2430,8 @@ connOkToAddRequest(ConnStateData::Pointer &conn) ssize_t ConnStateData::bodySizeLeft() { - if (body_reader != NULL) - return body_reader->remaining(); + if (body_reader_ != NULL) + return body_reader_->remaining(); return 0; } @@ -2519,27 +2541,43 @@ clientReadRequest(int fd, char *buf, size_t size, comm_err_t flag, int xerrno, if (flag == COMM_OK) { if (size > 0) { - /* - * If this assertion fails, we need to handle the case - * where a persistent connection has some leftover body - * request data from a previous, aborted transaction. - * Probably just decrement size and adjust/memmove the - * 'buf' pointer accordingly. - */ - assert(conn->in.abortedSize == 0); + kb_incr(&statCounter.client_http.kbytes_in, size); char *current_buf = conn->in.addressToReadInto(); - kb_incr(&statCounter.client_http.kbytes_in, size); if (buf != current_buf) xmemmove(current_buf, buf, size); - conn->in.notYetUsed += size; + conn->in.buf[conn->in.notYetUsed] = '\0'; /* Terminate the string */ - conn->in.buf[conn->in.notYetUsed] = '\0'; /* Terminate the string */ - - if (conn->body_reader != NULL) - conn->body_reader->notify(conn->in.notYetUsed); + /* if there is available non-aborted data, give it to the + * BodyReader + */ + if (conn->body_reader() != NULL) + conn->body_reader()->notify(conn->in.notYetUsed); + /* there is some aborted body to remove + * could we? should we? use BodyReader to eliminate this via an + * abort() api. + * + * This is not the most optimal path: ideally we would: + * - optimise the memmove above to not move data we're discarding + * - discard notYetUsed earlier + */ + if (conn->in.abortedSize) { + size_t discardSize = XMIN(conn->in.abortedSize, conn->in.notYetUsed); + /* these figures must match */ + assert(conn->in.abortedSize == (size_t)conn->bodySizeLeft()); + conn->body_reader()->reduce_remaining(discardSize); + connNoteUseOfBuffer(conn.getRaw(), discardSize); + conn->in.abortedSize -= discardSize; + if (!conn->in.abortedSize) + /* we've finished reading like good clients, + * now do the close that initiateClose initiated. + * + * XXX: do we have to close? why not check keepalive et. + */ + comm_close(fd); + } } else if (size == 0) { debug(33, 5) ("clientReadRequest: FD %d closed?\n", fd); @@ -3247,21 +3285,6 @@ clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http) CBDATA_CLASS_INIT(ConnStateData); -void * -ConnStateData::operator new (size_t) -{ - CBDATA_INIT_TYPE(ConnStateData); - ConnStateData *result = cbdataAlloc(ConnStateData); - return result; -} - -void -ConnStateData::operator delete (void *address) -{ - ConnStateData *t = static_cast(address); - cbdataFree(t); -} - ConnStateData::ConnStateData() : transparent_ (false), reading_ (false), closing_ (false) { openReference = this; diff --git a/src/client_side.h b/src/client_side.h index a100aead7d..42e076522b 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -1,6 +1,6 @@ /* - * $Id: client_side.h,v 1.17 2006/08/07 02:28:22 robertc Exp $ + * $Id: client_side.h,v 1.18 2006/09/02 06:49:48 robertc Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -126,13 +126,13 @@ private: bool connRegistered_; }; + +/* A connection to a socket */ class ConnStateData : public RefCountable { public: typedef RefCount Pointer; - void * operator new (size_t); - void operator delete (void *); ConnStateData(); ~ConnStateData(); @@ -168,11 +168,8 @@ public: * will be staying open. */ size_t abortedSize; - } - - in; + } in; - BodyReader::Pointer body_reader; ssize_t bodySizeLeft(); /* @@ -214,12 +211,21 @@ public: bool closing() const; void closing(bool const); + /* get the body reader that has been attached to the client + * request + */ + BodyReader * body_reader(); + /* set a body reader that should read data from the request + */ + void body_reader(BodyReader::Pointer); + private: - CBDATA_CLASS(ConnStateData); + CBDATA_CLASS2(ConnStateData); bool transparent_; bool reading_; bool closing_; Pointer openReference; + BodyReader::Pointer body_reader_; }; /* convenience class while splitting up body handling */ diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 0193135134..4379f170d8 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side_request.cc,v 1.71 2006/06/21 22:37:49 wessels Exp $ + * $Id: client_side_request.cc,v 1.72 2006/09/02 06:49:48 robertc Exp $ * * DEBUG: section 85 Client-side Request Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -129,7 +129,7 @@ ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(anH redirect_done = false; no_cache_done = false; interpreted_req_hdrs = false; - debugs(85,3, HERE << this << " ClientHttpRequest constructed"); + debugs(85,3, HERE << this << " ClientRequestContext constructed"); } CBDATA_CLASS_INIT(ClientHttpRequest); @@ -242,17 +242,15 @@ ClientHttpRequest::~ClientHttpRequest() */ if (request && request->body_reader != NULL) { - request->body_reader = NULL; // refcounted - debugs(32,3,HERE << "setting body_reader = NULL for request " << request); + request->body_reader = NULL; // refcounted, triggers abort if needed. + debugs(32, 3, HERE << "setting body_reader = NULL for request " << request); } /* the ICP check here was erroneous * - storeReleaseRequest was always called if entry was valid */ assert(logType < LOG_TYPE_MAX); - logRequest(); - loggingEntry(NULL); if (request) @@ -261,12 +259,10 @@ ClientHttpRequest::~ClientHttpRequest() freeResources(); #if ICAP_CLIENT - if (icap) { delete icap; cbdataReferenceDone(icap); } - #endif if (calloutContext) delete calloutContext; @@ -1085,14 +1081,14 @@ ClientHttpRequest::doCallouts() int ClientHttpRequest::doIcap(ICAPServiceRep::Pointer service) { - debugs(85,3, HERE << this << " ClientHttpRequest::doIcap() called"); + debugs(85, 3, HERE << this << " ClientHttpRequest::doIcap() called"); assert(NULL == icap); icap = new ICAPClientReqmodPrecache(service); (void) cbdataReference(icap); icap->startReqMod(this, request); if (request->body_reader == NULL) { - debugs(32,3,HERE << "client request hasnt body..."); + debugs(32, 3, HERE << "client request hasnt body..."); icap->doneSending(); } diff --git a/src/errorpage.cc b/src/errorpage.cc index e5997ee9e5..96b340dd03 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -1,6 +1,6 @@ /* - * $Id: errorpage.cc,v 1.217 2006/08/25 15:22:34 serassio Exp $ + * $Id: errorpage.cc,v 1.218 2006/09/02 06:49:48 robertc Exp $ * * DEBUG: section 4 Error Generation * AUTHOR: Duane Wessels @@ -357,8 +357,12 @@ errorAppendEntry(StoreEntry * entry, ErrorState * err) HttpReply *rep; assert(entry->mem_obj != NULL); assert (entry->isEmpty()); + debugs(4, 4, "Creating an error page for entry " << entry << + " with errorstate " << err << + " page id " << err->page_id); if (entry->store_status != STORE_PENDING) { + debugs(4, 2, "Skipping error page due to store_status: " << entry->store_status); /* * If the entry is not STORE_PENDING, then no clients * care about it, and we don't need to generate an @@ -377,34 +381,23 @@ errorAppendEntry(StoreEntry * entry, ErrorState * err) } } - entry->lock() - - ; + entry->lock(); storeBuffer(entry); - rep = errorBuildReply(err); - /* Add authentication header */ /* TODO: alter errorstate to be accel on|off aware. The 0 on the next line - * depends on authenticate behaviour: all schemes to date send no extra data - * on 407/401 responses, and do not check the accel state on 401/407 responses + * depends on authenticate behaviour: all schemes to date send no extra + * data on 407/401 responses, and do not check the accel state on 401/407 + * responses */ authenticateFixHeader(rep, err->auth_user_request, err->request, 0, 1); - entry->replaceHttpReply(rep); - EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT); - storeBufferFlush(entry); - entry->complete(); - storeNegativeCache(entry); - storeReleaseRequest(entry); - entry->unlock(); - errorStateFree(err); }