From: Dmitry Kurochkin Date: Thu, 18 Apr 2013 00:23:37 +0000 (+0400) Subject: FTP gateway: process FTP request asynchronously. X-Git-Tag: SQUID_3_5_0_1~117^2~69 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c8059ea9de1b8870297ca133dfb5184218711160;p=thirdparty%2Fsquid.git FTP gateway: process FTP request asynchronously. Before the change, FTP request processing was done right after parsing. This could result in data transfer request failure because data connection accept callback was already scheduled but not fired yet. The patch makes FTP request processing asynchronous to fix this race. --- diff --git a/src/FtpGatewayServer.cc b/src/FtpGatewayServer.cc index d5d90be202..9ad7575f30 100644 --- a/src/FtpGatewayServer.cc +++ b/src/FtpGatewayServer.cc @@ -340,19 +340,21 @@ ServerStateData::sendCommand() return; } - String cmd = fwd->request->header.findEntry(HDR_FTP_COMMAND)->value; - const String *const params = fwd->request->header.has(HDR_FTP_ARGUMENTS) ? - &fwd->request->header.findEntry(HDR_FTP_ARGUMENTS)->value : NULL; - - if (params != NULL) - debugs(9, 5, HERE << "command: " << cmd << ", parameters: " << *params); + HttpHeader &header = fwd->request->header; + assert(header.has(HDR_FTP_COMMAND)); + const String &cmd = header.findEntry(HDR_FTP_COMMAND)->value; + assert(header.has(HDR_FTP_ARGUMENTS)); + const String ¶ms = header.findEntry(HDR_FTP_ARGUMENTS)->value; + + if (params.size() > 0) + debugs(9, 5, HERE << "command: " << cmd << ", parameters: " << params); else debugs(9, 5, HERE << "command: " << cmd << ", no parameters"); static MemBuf mb; mb.reset(); - if (params != NULL) - mb.Printf("%s %s%s", cmd.termedBuf(), params->termedBuf(), Ftp::crlf); + if (params.size() > 0) + mb.Printf("%s %s%s", cmd.termedBuf(), params.termedBuf(), Ftp::crlf); else mb.Printf("%s%s", cmd.termedBuf(), Ftp::crlf); diff --git a/src/client_side.cc b/src/client_side.cc index f45d804a26..2c0b7b24d8 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -105,6 +105,7 @@ #include "fqdncache.h" #include "globals.h" #include "http.h" +#include "HttpHdrCc.h" #include "HttpHdrContRange.h" #include "HttpHeaderTools.h" #include "HttpReply.h" @@ -253,6 +254,7 @@ static IOACB FtpAcceptDataConnection; static void FtpCloseDataConnection(ConnStateData *conn); static void FtpWriteGreeting(ConnStateData *conn); static ClientSocketContext *FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::ProtocolVersion *http_ver); +static bool FtpHandleUserRequest(ConnStateData *connState, const String &cmd, String ¶ms); static void FtpHandleReply(ClientSocketContext *context, HttpReply *reply, StoreIOBuffer data); typedef void FtpReplyHandler(ClientSocketContext *context, const HttpReply *reply, StoreIOBuffer data); @@ -270,14 +272,14 @@ static IOCB FtpWroteEarlyReply; static IOCB FtpWroteReply; static IOCB FtpWroteReplyData; -typedef bool FtpRequestHandler(ConnStateData *connState, String &cmd, String ¶ms); +typedef bool FtpRequestHandler(ClientSocketContext *context, String &cmd, String ¶ms); static FtpRequestHandler FtpHandleRequest; -static FtpRequestHandler FtpHandleUserRequest; static FtpRequestHandler FtpHandlePasvRequest; static FtpRequestHandler FtpHandlePortRequest; static FtpRequestHandler FtpHandleDataRequest; -static bool FtpCheckDataConnection(ConnStateData *connState); +static bool FtpCheckDataConnection(ClientSocketContext *context); +static void FtpSetReply(ClientSocketContext *context, const int code, const char *msg); clientStreamNode * ClientSocketContext::getTail() const @@ -2649,7 +2651,8 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c /* We have an initial client stream in place should it be needed */ /* setup our private context */ - context->registerWithConn(); + if (!conn->isFtp) + context->registerWithConn(); if (context->flags.parsed_ok == 0) { clientStreamNode *node = context->getClientReplyContext(); @@ -2679,6 +2682,7 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c if (conn->isFtp) { assert(http->request); request = http->request; + notedUseOfBuffer = true; } else if ((request = HttpRequest::CreateFromUrlAndMethod(http->uri, method)) == NULL) { clientStreamNode *node = context->getClientReplyContext(); @@ -2862,9 +2866,11 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c request->body_pipe = conn->expectRequestBody( chunked ? -1 : request->content_length); - // consume header early so that body pipe gets just the body - connNoteUseOfBuffer(conn, http->req_sz); - notedUseOfBuffer = true; + if (!notedUseOfBuffer) { + // consume header early so that body pipe gets just the body + connNoteUseOfBuffer(conn, http->req_sz); + notedUseOfBuffer = true; + } /* Is it too large? */ if (!chunked && // if chunked, we will check as we accumulate @@ -2915,6 +2921,31 @@ finish: } } +void +ConnStateData::processFtpRequest(ClientSocketContext *const context) +{ + ClientHttpRequest *const http = context->http; + assert(http != NULL); + HttpRequest *const request = http->request; + assert(request != NULL); + HttpHeader &header = request->header; + assert(header.has(HDR_FTP_COMMAND)); + String &cmd = header.findEntry(HDR_FTP_COMMAND)->value; + assert(header.has(HDR_FTP_ARGUMENTS)); + String ¶ms = header.findEntry(HDR_FTP_ARGUMENTS)->value; + + if (!FtpHandleRequest(context, cmd, params)) { + assert(http->storeEntry() != NULL); + clientSetKeepaliveFlag(http); + context->pullData(); + return; + } + + assert(http->storeEntry() == NULL); + clientProcessRequest(this, &parser_, context, request->method, + request->http_ver); +} + static void connStripBufferWhitespace (ConnStateData * conn) { @@ -2997,7 +3028,14 @@ ConnStateData::clientParseRequests() CommTimeoutCbPtrFun(clientLifetimeTimeout, context->http)); commSetConnTimeout(clientConnection, Config.Timeout.lifetime, timeoutCall); - clientProcessRequest(this, &parser_, context, method, http_ver); + if (!isFtp) + clientProcessRequest(this, &parser_, context, method, http_ver); + else { + // Process FTP request asynchronously to make sure FTP + // data connection accept callback is fired first. + CallJobHere1(33, 4, CbcPointer(this), + ConnStateData, ConnStateData::processFtpRequest, context); + } parsed_req = true; // XXX: do we really need to parse everything right NOW ? @@ -4820,6 +4858,7 @@ FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::Pro const char *const eor = static_cast(memchr(connState->in.buf, '\n', min(connState->in.notYetUsed, Config.maxRequestHeaderSize))); + const size_t req_sz = eor + 1 - connState->in.buf; if (eor == NULL && connState->in.notYetUsed >= Config.maxRequestHeaderSize) { connState->ftp.state = ConnStateData::FTP_ERROR; @@ -4832,12 +4871,13 @@ FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::Pro return NULL; } + connNoteUseOfBuffer(connState, req_sz); + // skip leading whitespaces const char *boc = connState->in.buf; while (boc < eor && isspace(*boc)) ++boc; if (boc >= eor) { debugs(33, 5, HERE << "Empty request, ignoring"); - connNoteUseOfBuffer(connState, eor + 1 - connState->in.buf); return NULL; } @@ -4859,23 +4899,28 @@ FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::Pro (bop == NULL ? "no " : "") << "parameters" << (bop != NULL ? ": " : "") << bop); - String cmd = boc; + const String cmd = boc; String params = bop; - // the first command must be USER - if (connState->ftp.uri.size() == 0 && cmd.caseCmp("USER") != 0) { - debugs(33, 5, HERE << "Unexpected FTP command: expected USER, got " << - boc); - FtpWriteEarlyReply(connState, 530, "Must login first"); - connNoteUseOfBuffer(connState, eor + 1 - connState->in.buf); - return NULL; + if (connState->ftp.uri.size() == 0) { + // the first command must be USER + if (cmd.caseCmp("USER") != 0) { + FtpWriteEarlyReply(connState, 530, "Must login first"); + return NULL; + } + + if (params.size() == 0) { + FtpWriteEarlyReply(connState, 501, "Missing username"); + return NULL; + } } - if (!FtpHandleRequest(connState, cmd, params)) { - connNoteUseOfBuffer(connState, eor + 1 - connState->in.buf); + // We need to process USER request now because it sets request URI. + if (cmd.caseCmp("USER") == 0 && + !FtpHandleUserRequest(connState, cmd, params)) return NULL; - } + assert(connState->ftp.uri.size() > 0); char *uri = xstrdup(connState->ftp.uri.termedBuf()); HttpRequest *const request = HttpRequest::CreateFromUrlAndMethod(uri, *method_p); @@ -4883,20 +4928,20 @@ FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::Pro debugs(33, 5, HERE << "Invalid FTP URL: " << connState->ftp.uri); FtpWriteEarlyReply(connState, 501, "Invalid host"); connState->ftp.uri.clean(); - connNoteUseOfBuffer(connState, eor + 1 - connState->in.buf); safe_free(uri); return NULL; } + request->http_ver = *http_ver; request->header.putStr(HDR_FTP_COMMAND, cmd.termedBuf()); - if (params.size() > 0) - request->header.putStr(HDR_FTP_ARGUMENTS, params.termedBuf()); + request->header.putStr(HDR_FTP_ARGUMENTS, params.termedBuf() != NULL ? + params.termedBuf() : ""); ClientHttpRequest *const http = new ClientHttpRequest(connState); http->request = request; HTTPMSGLOCK(http->request); - http->req_sz = eor - connState->in.buf + 1; - http->uri = xstrdup(connState->ftp.uri.termedBuf()); + http->req_sz = req_sz; + http->uri = uri; ClientSocketContext *const result = ClientSocketContextNew(connState->clientConnection, http); @@ -4911,6 +4956,7 @@ FtpParseRequest(ConnStateData *connState, HttpRequestMethod *method_p, Http::Pro clientReplyStatus, newServer, clientSocketRecipient, clientSocketDetach, newClient, tempBuffer); + result->registerWithConn(); result->flags.parsed_ok = 1; connState->flags.readMore = false; return result; @@ -5185,14 +5231,13 @@ FtpWroteReply(const Comm::ConnectionPointer &conn, char *bufnotused, size_t size } bool -FtpHandleRequest(ConnStateData *connState, String &cmd, String ¶ms) { +FtpHandleRequest(ClientSocketContext *context, String &cmd, String ¶ms) { static std::pair handlers[] = { std::make_pair("PASV", FtpHandlePasvRequest), std::make_pair("PORT", FtpHandlePortRequest), std::make_pair("RETR", FtpHandleDataRequest), std::make_pair("LIST", FtpHandleDataRequest), - std::make_pair("NLST", FtpHandleDataRequest), - std::make_pair("USER", FtpHandleUserRequest) + std::make_pair("NLST", FtpHandleDataRequest) }; FtpRequestHandler *handler = NULL; @@ -5203,11 +5248,11 @@ FtpHandleRequest(ConnStateData *connState, String &cmd, String ¶ms) { } } - return handler != NULL ? (*handler)(connState, cmd, params) : true; + return handler != NULL ? (*handler)(context, cmd, params) : true; } bool -FtpHandleUserRequest(ConnStateData *connState, String &cmd, String ¶ms) +FtpHandleUserRequest(ConnStateData *connState, const String &cmd, String ¶ms) { if (params.size() == 0) { FtpWriteEarlyReply(connState, 501, "Missing username"); @@ -5239,45 +5284,77 @@ FtpHandleUserRequest(ConnStateData *connState, String &cmd, String ¶ms) } bool -FtpHandlePasvRequest(ConnStateData *connState, String &cmd, String ¶ms) +FtpHandlePasvRequest(ClientSocketContext *context, String &cmd, String ¶ms) { if (params.size() > 0) { - FtpWriteEarlyReply(connState, 501, "Unexpected parameter"); + FtpSetReply(context, 501, "Unexpected parameter"); return false; } - connState->ftp.state = ConnStateData::FTP_HANDLE_PASV; + context->getConn()->ftp.state = ConnStateData::FTP_HANDLE_PASV; return true; } bool -FtpHandlePortRequest(ConnStateData *connState, String &cmd, String ¶ms) +FtpHandlePortRequest(ClientSocketContext *context, String &cmd, String ¶ms) { - FtpWriteEarlyReply(connState, 502, "Command not supported"); + FtpSetReply(context, 502, "Command not supported"); return false; } bool -FtpHandleDataRequest(ConnStateData *connState, String &cmd, String ¶ms) +FtpHandleDataRequest(ClientSocketContext *context, String &cmd, String ¶ms) { - if (!FtpCheckDataConnection(connState)) + if (!FtpCheckDataConnection(context)) return false; - connState->ftp.state = ConnStateData::FTP_HANDLE_DATA_REQUEST; + context->getConn()->ftp.state = ConnStateData::FTP_HANDLE_DATA_REQUEST; return true; } bool -FtpCheckDataConnection(ConnStateData *connState) +FtpCheckDataConnection(ClientSocketContext *context) { + const ConnStateData *const connState = context->getConn(); if (Comm::IsConnOpen(connState->ftp.dataConn)) return true; if (!Comm::IsConnOpen(connState->ftp.dataListenConn)) - FtpWriteEarlyReply(connState, 425, "Use PASV first"); + FtpSetReply(context, 425, "Use PASV first"); else - FtpWriteEarlyReply(connState, 425, "Data connection is not established"); + FtpSetReply(context, 425, "Data connection is not established"); return false; } + +void +FtpSetReply(ClientSocketContext *context, const int code, const char *msg) +{ + ClientHttpRequest *const http = context->http; + assert(http != NULL); + assert(http->storeEntry() == NULL); + + HttpReply *const reply = new HttpReply; + reply->sline.set(Http::ProtocolVersion(1, 1), Http::scNoContent); + HttpHeader &header = reply->header; + header.putTime(HDR_DATE, squid_curtime); + { + HttpHdrCc cc; + cc.Private(); + header.putCc(&cc); + } + header.putInt64(HDR_CONTENT_LENGTH, 0); + header.putInt(HDR_FTP_STATUS, code); + header.putStr(HDR_FTP_REASON, msg); + reply->hdrCacheInit(); + + setLogUri(http, http->uri, true); + + clientStreamNode *const node = context->getClientReplyContext(); + clientReplyContext *const repContext = + dynamic_cast(node->data.getRaw()); + assert(repContext != NULL); + repContext->createStoreEntry(http->request->method, RequestFlags()); + http->storeEntry()->replaceHttpReply(reply); +} diff --git a/src/client_side.h b/src/client_side.h index c75a3026d4..7d7d85af53 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -399,6 +399,7 @@ private: int connReadWasError(comm_err_t flag, int size, int xerrno); int connFinishedWithConn(int size); void clientAfterReadingRequests(); + void processFtpRequest(ClientSocketContext *const context); private: HttpParser parser_;