From: Alex Rousskov Date: Fri, 3 Feb 2012 18:01:27 +0000 (-0700) Subject: Disable persistent connections after client-side-detected errors. X-Git-Tag: BumpSslServerFirst.take05~29 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=84c777488c4eed80d3fdb73f89268574e2b0a9f8;p=thirdparty%2Fsquid.git Disable persistent connections after client-side-detected errors. HTTP does not require to close the connection after most errors, but some client-side-detected errors (those detected by clientProcessRequest) are about HTTP message framing and should result in connection closure. For other client-side-detected errors (those detected in serveDelayedError), the client-side code is just not ready to handle persistency well: * If we leave flags.readMore as true, then a new request on the same connection may find bumpErrorEntry set and think there is a delayed error that needs to be served, but that StoreEntry object has been already used. And we should not simply clear bumpErrorEntry because it is not locked by the error writing code using it. There may be other problems as well (and it is likely the connection is not usable anyway because the fake certificate was rejected by the client). * If we set flags.readMore to false then we will not resume reading after serving the error, causing "abandoning such and such connection" errors and stuck ConnStateData jobs. Thus, in summary, we should set flags.readMore to false and, if we do that, we should also set proxy_keepalive to zero so that we close the connection after serving the error to the client. --- diff --git a/src/client_side.cc b/src/client_side.cc index 5ca08665e1..f3a2530be8 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2447,6 +2447,18 @@ ConnStateData::clientAfterReadingRequests() readSomeData(); } +void +ConnStateData::quitAfterError(HttpRequest *request) +{ + // From HTTP p.o.v., we do not have to close after every error detected + // at the client-side, but many such errors do require closure and the + // client-side code is bad at handling errors so we play it safe. + if (request) + request->flags.proxy_keepalive = 0; + flags.readMore = false; + debugs(33,4, HERE << "Will close after error: " << clientConnection); +} + #if USE_SSL bool ConnStateData::serveDelayedError(ClientSocketContext *context) { @@ -2456,6 +2468,8 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) return false; if (!e->isEmpty()) { + quitAfterError(http->request); + //Failed? Here we should get the error from conn and send it to client // The error stored in ConnStateData::bumpFirstEntry, replace the // ClientHttpRequest store entry with this. @@ -2465,7 +2479,6 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) debugs(33, 5, "Connection first has failed for " << http->uri << ". Respond with an error"); repContext->setReplyToStoreEntry(e); context->pullData(); - flags.readMore = false; return true; } @@ -2485,6 +2498,8 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) check.sslErrorList = NULL; if (!allowDomainMismatch) { + quitAfterError(request); + clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); @@ -2502,7 +2517,6 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) repContext->setReplyToError(request->method, err); assert(context->http->out.offset == 0); context->pullData(); - flags.readMore = false; return true; } } @@ -2530,6 +2544,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c if (context->flags.parsed_ok == 0) { clientStreamNode *node = context->getClientReplyContext(); debugs(33, 2, "clientProcessRequest: Invalid Request"); + conn->quitAfterError(NULL); // setLogUri should called before repContext->setReplyToError setLogUri(http, http->uri, true); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); @@ -2548,13 +2563,13 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c } assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } if ((request = HttpRequest::CreateFromUrlAndMethod(http->uri, method)) == NULL) { clientStreamNode *node = context->getClientReplyContext(); debugs(33, 5, "Invalid URL: " << http->uri); + conn->quitAfterError(request); // setLogUri should called before repContext->setReplyToError setLogUri(http, http->uri, true); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); @@ -2562,7 +2577,6 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c repContext->setReplyToError(ERR_INVALID_URL, HTTP_BAD_REQUEST, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } @@ -2574,6 +2588,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c clientStreamNode *node = context->getClientReplyContext(); debugs(33, 5, "Unsupported HTTP version discovered. :\n" << HttpParserHdrBuf(hp)); + conn->quitAfterError(request); // setLogUri should called before repContext->setReplyToError setLogUri(http, http->uri, true); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); @@ -2582,7 +2597,6 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c conn->clientConnection->remote, NULL, HttpParserHdrBuf(hp), NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } @@ -2592,6 +2606,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c if (http_ver.major >= 1 && !request->parseHeader(HttpParserHdrBuf(hp), HttpParserHdrSz(hp))) { clientStreamNode *node = context->getClientReplyContext(); debugs(33, 5, "Failed to parse request headers:\n" << HttpParserHdrBuf(hp)); + conn->quitAfterError(request); // setLogUri should called before repContext->setReplyToError setLogUri(http, http->uri, true); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); @@ -2599,7 +2614,6 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c repContext->setReplyToError(ERR_INVALID_REQ, HTTP_BAD_REQUEST, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } @@ -2665,13 +2679,13 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c (request->header.getInt64(HDR_MAX_FORWARDS) == 0); if (!urlCheckRequest(request) || mustReplyToOptions || unsupportedTe) { clientStreamNode *node = context->getClientReplyContext(); + conn->quitAfterError(request); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_UNSUP_REQ, HTTP_NOT_IMPLEMENTED, request->method, NULL, conn->clientConnection->remote, request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } @@ -2680,12 +2694,12 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); + conn->quitAfterError(request); repContext->setReplyToError(ERR_INVALID_REQ, HTTP_LENGTH_REQUIRED, request->method, NULL, conn->clientConnection->remote, request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } @@ -2696,11 +2710,11 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); + conn->quitAfterError(request); repContext->setReplyToError(ERR_INVALID_REQ, HTTP_EXPECTATION_FAILED, request->method, http->uri, conn->clientConnection->remote, request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } } @@ -2735,12 +2749,12 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); + conn->quitAfterError(request); repContext->setReplyToError(ERR_TOO_BIG, HTTP_REQUEST_ENTITY_TOO_LARGE, METHOD_NONE, NULL, conn->clientConnection->remote, http->request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - conn->flags.readMore = false; goto finish; } diff --git a/src/client_side.h b/src/client_side.h index 65c4b1b2a6..60753dabaa 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -312,6 +312,10 @@ public: virtual bool doneAll() const { return BodyProducer::doneAll() && false;} virtual void swanSong(); + /// Changes state so that we close the connection and quit after serving + /// the client-side-detected error response instead of getting stuck. + void quitAfterError(HttpRequest *request); // meant to be private + #if USE_SSL /// called by Ssl::ServerPeeker when it is done bumping the server void httpsPeeked(Comm::ConnectionPointer serverConnection);