------------------------------------------------------------ revno: 13543 revision-id: squid3@treenet.co.nz-20140823110540-dyjakp0otgwitqol parent: squid3@treenet.co.nz-20140822141355-t2490vsglnk5f233 committer: Amos Jeffries branch nick: trunk timestamp: Sat 2014-08-23 04:05:40 -0700 message: Cleanup: remove goto from clientProcessRequest() The code path can be better statically analysed without goto statements. Bonus side effect is that several cases of checking for cleanup code can be removed entirely. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20140823110540-dyjakp0otgwitqol # target_branch: http://bzr.squid-cache.org/bzr/squid3/trunk/ # testament_sha1: 6314a0ed6beba16b263338a32894cf18f2880047 # timestamp: 2014-08-23 11:54:05 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/trunk/ # base_revision_id: squid3@treenet.co.nz-20140822141355-\ # t2490vsglnk5f233 # # Begin patch === modified file 'src/client_side.cc' --- src/client_side.cc 2014-08-19 18:09:50 +0000 +++ src/client_side.cc 2014-08-23 11:05:40 +0000 @@ -2524,6 +2524,23 @@ } #endif // USE_OPENSSL +static void +clientProcessRequestFinished(ConnStateData *conn, const HttpRequest::Pointer &request) +{ + /* + * DPW 2007-05-18 + * Moved the TCP_RESET feature from clientReplyContext::sendMoreData + * to here because calling comm_reset_close() causes http to + * be freed and the above connNoteUseOfBuffer() would hit an + * assertion, not to mention that we were accessing freed memory. + */ + if (request != NULL && request->flags.resetTcp && Comm::IsConnOpen(conn->clientConnection)) { + debugs(33, 3, HERE << "Sending TCP RST on " << conn->clientConnection); + conn->flags.readMore = false; + comm_reset_close(conn->clientConnection); + } +} + void clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *context, const HttpRequestMethod& method, Http::ProtocolVersion http_ver) { @@ -2543,87 +2560,91 @@ assert(http->request); request = http->request; notedUseOfBuffer = true; - goto doFtpAndHttp; - } - - if (context->flags.parsed_ok == 0) { - assert(hp); - 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()); - assert (repContext); - switch (hp->request_parse_status) { - case Http::scHeaderTooLarge: - repContext->setReplyToError(ERR_TOO_BIG, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL); - break; - case Http::scMethodNotAllowed: - repContext->setReplyToError(ERR_UNSUP_REQ, Http::scMethodNotAllowed, method, http->uri, - conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL); - break; - default: - repContext->setReplyToError(ERR_INVALID_REQ, hp->request_parse_status, method, http->uri, - conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL); - } - assert(context->http->out.offset == 0); - context->pullData(); - goto finish; - } - - if ((request = HttpRequest::CreateFromUrlAndMethod(http->uri, method)) == NULL) { - clientStreamNode *node = context->getClientReplyContext(); - debugs(33, 5, "Invalid URL: " << http->uri); - conn->quitAfterError(request.getRaw()); - // setLogUri should called before repContext->setReplyToError - setLogUri(http, http->uri, true); - clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); - assert (repContext); - repContext->setReplyToError(ERR_INVALID_URL, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL); - assert(context->http->out.offset == 0); - context->pullData(); - goto finish; - } - - /* RFC 2616 section 10.5.6 : handle unsupported HTTP major versions cleanly. */ - /* We currently only support 0.9, 1.0, 1.1 properly */ - /* TODO: move HTTP-specific processing into servers/HttpServer and such */ - if ( (http_ver.major == 0 && http_ver.minor != 9) || - (http_ver.major > 1) ) { - - clientStreamNode *node = context->getClientReplyContext(); - debugs(33, 5, "Unsupported HTTP version discovered. :\n" << HttpParserHdrBuf(hp)); - conn->quitAfterError(request.getRaw()); - // setLogUri should called before repContext->setReplyToError - setLogUri(http, http->uri, true); - clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); - assert (repContext); - repContext->setReplyToError(ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, method, http->uri, - conn->clientConnection->remote, NULL, HttpParserHdrBuf(hp), NULL); - assert(context->http->out.offset == 0); - context->pullData(); - goto finish; - } - - /* compile headers */ - /* we should skip request line! */ - /* XXX should actually know the damned buffer size here */ - 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.getRaw()); - // setLogUri should called before repContext->setReplyToError - setLogUri(http, http->uri, true); - clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); - assert (repContext); - repContext->setReplyToError(ERR_INVALID_REQ, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL); - assert(context->http->out.offset == 0); - context->pullData(); - goto finish; - } - -doFtpAndHttp: + } else { + + 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()); + assert (repContext); + switch (hp->request_parse_status) { + case Http::scHeaderTooLarge: + repContext->setReplyToError(ERR_TOO_BIG, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL); + break; + case Http::scMethodNotAllowed: + repContext->setReplyToError(ERR_UNSUP_REQ, Http::scMethodNotAllowed, method, http->uri, + conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL); + break; + default: + repContext->setReplyToError(ERR_INVALID_REQ, hp->request_parse_status, method, http->uri, + conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL); + } + assert(context->http->out.offset == 0); + context->pullData(); + connNoteUseOfBuffer(conn, http->req_sz); + return; + } + + if ((request = HttpRequest::CreateFromUrlAndMethod(http->uri, method)) == NULL) { + clientStreamNode *node = context->getClientReplyContext(); + debugs(33, 5, "Invalid URL: " << http->uri); + conn->quitAfterError(request.getRaw()); + // setLogUri should called before repContext->setReplyToError + setLogUri(http, http->uri, true); + clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); + assert (repContext); + repContext->setReplyToError(ERR_INVALID_URL, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL); + assert(context->http->out.offset == 0); + context->pullData(); + connNoteUseOfBuffer(conn, http->req_sz); + return; + } + + /* RFC 2616 section 10.5.6 : handle unsupported HTTP major versions cleanly. */ + /* We currently only support 0.9, 1.0, 1.1 properly */ + /* TODO: move HTTP-specific processing into servers/HttpServer and such */ + if ( (http_ver.major == 0 && http_ver.minor != 9) || + (http_ver.major > 1) ) { + + clientStreamNode *node = context->getClientReplyContext(); + debugs(33, 5, "Unsupported HTTP version discovered. :\n" << HttpParserHdrBuf(hp)); + conn->quitAfterError(request.getRaw()); + // setLogUri should called before repContext->setReplyToError + setLogUri(http, http->uri, true); + clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); + assert (repContext); + repContext->setReplyToError(ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, method, http->uri, + conn->clientConnection->remote, NULL, HttpParserHdrBuf(hp), NULL); + assert(context->http->out.offset == 0); + context->pullData(); + connNoteUseOfBuffer(conn, http->req_sz); + clientProcessRequestFinished(conn, request); + return; + } + + /* compile headers */ + /* we should skip request line! */ + /* XXX should actually know the damned buffer size here */ + 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.getRaw()); + // setLogUri should called before repContext->setReplyToError + setLogUri(http, http->uri, true); + clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); + assert (repContext); + repContext->setReplyToError(ERR_INVALID_REQ, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL); + assert(context->http->out.offset == 0); + context->pullData(); + connNoteUseOfBuffer(conn, http->req_sz); + clientProcessRequestFinished(conn, request); + return; + } + } + // Some blobs below are still HTTP-specific, but we would have to rewrite // this entire function to remove them from the FTP code path. Connection // setup and body_pipe preparation blobs are needed for FTP. @@ -2715,7 +2736,9 @@ conn->clientConnection->remote, request.getRaw(), NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - goto finish; + connNoteUseOfBuffer(conn, http->req_sz); + clientProcessRequestFinished(conn, request); + return; } if (!chunked && !clientIsContentLengthValid(request.getRaw())) { @@ -2728,7 +2751,9 @@ conn->clientConnection->remote, request.getRaw(), NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - goto finish; + connNoteUseOfBuffer(conn, http->req_sz); + clientProcessRequestFinished(conn, request); + return; } if (request->header.has(HDR_EXPECT)) { @@ -2743,7 +2768,9 @@ conn->clientConnection->remote, request.getRaw(), NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - goto finish; + connNoteUseOfBuffer(conn, http->req_sz); + clientProcessRequestFinished(conn, request); + return; } } @@ -2764,8 +2791,12 @@ } #if USE_OPENSSL - if (conn->switchedToHttps() && conn->serveDelayedError(context)) - goto finish; + if (conn->switchedToHttps() && conn->serveDelayedError(context)) { + if (!notedUseOfBuffer) + connNoteUseOfBuffer(conn, http->req_sz); + clientProcessRequestFinished(conn, request); + return; + } #endif /* Do we expect a request-body? */ @@ -2792,14 +2823,17 @@ conn->clientConnection->remote, http->request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); - goto finish; + clientProcessRequestFinished(conn, request); + return; } if (!isFtp) { // We may stop producing, comm_close, and/or call setReplyToError() // below, so quit on errors to avoid http->doCallouts() - if (!conn->handleRequestBodyData()) - goto finish; + if (!conn->handleRequestBodyData()) { + clientProcessRequestFinished(conn, request); + return; + } if (!request->body_pipe->productionEnded()) { debugs(33, 5, "need more request body"); @@ -2813,22 +2847,10 @@ http->doCallouts(); -finish: if (!notedUseOfBuffer) connNoteUseOfBuffer(conn, http->req_sz); - /* - * DPW 2007-05-18 - * Moved the TCP_RESET feature from clientReplyContext::sendMoreData - * to here because calling comm_reset_close() causes http to - * be freed and the above connNoteUseOfBuffer() would hit an - * assertion, not to mention that we were accessing freed memory. - */ - if (request != NULL && request->flags.resetTcp && Comm::IsConnOpen(conn->clientConnection)) { - debugs(33, 3, HERE << "Sending TCP RST on " << conn->clientConnection); - conn->flags.readMore = false; - comm_reset_close(conn->clientConnection); - } + clientProcessRequestFinished(conn, request); } static void