Compliance: Forward 1xx control messages to clients that support them. Take 0, which needs more work. The patch removes ignore_expect_100 squid.conf option because we can safely forward Expect: 100-continue headers to servers because we can forward 100 Continue control messages to the expecting clients. We now forward 1xx control messages to all HTTP/1.1 clients and to HTTP/1.0 clients that sent an Expect: 100-continue header. RFC 2616 requires clients to accept 1xx control messages, even if they did not send Expect headers. We still respond with 417 Expectation Failed to requests with expectations other than 100-continue. Implementation notes: We forward control messages one-at-a-time and stop processing the server response while the 1xx message is being written to client, to avoid server-driven DoS attacks with large number of 1xx messages. 1xx forwarding is done via async calls from HttpStateData to ConnStateData/ClientSocketContext. The latter then calls back to notify HttpStateData that the message was written out to client. If any one of the two async messages is not fired, HttpStateData will get stuck unless it is destroyed due to an external event/error. The code assumes such event/error will always happen because when ConnStateData/ClientSocketContext is gone, HttpStateData job should be terminated. This requires more testing/thought. XXX: The patch is not finished. We need to cbdata-protect the HttpRequest::clientConnection member and re-sync with trunk. === added file 'src/HttpControlMsg.h' --- src/HttpControlMsg.h 1970-01-01 00:00:00 +0000 +++ src/HttpControlMsg.h 2010-08-18 19:36:04 +0000 @@ -0,0 +1,57 @@ +/* + * $Id$ + */ + +#ifndef SQUID_HTTP_CONTROL_MSG_H +#define SQUID_HTTP_CONTROL_MSG_H + +#include "HttpReply.h" +#include "base/AsyncCall.h" + +class HttpControlMsg; + +/* + * This API exists to throttle forwarding of 1xx messages from the server + * side (Source == HttpStateData) to the client side (Sink == ConnStateData). + * + * Without throttling, Squid would have to drop some 1xx responses to + * avoid DoS attacks that send many 1xx responses without reading them. + * Dropping 1xx responses without violating HTTP is as complex as throttling. + */ + +/// sends a single control message, notifying the Sink +class HttpControlMsgSink: public virtual AsyncJob +{ +public: + HttpControlMsgSink(): AsyncJob("unused") {} + + /// called to send the 1xx message and notify the Source + virtual void sendControlMsg(HttpControlMsg msg) = 0; +}; + +/// bundles HTTP 1xx reply and the "successfully forwarded" callback +class HttpControlMsg +{ +public: + typedef HttpMsgPointerT MsgPtr; + typedef AsyncCall::Pointer Callback; + + HttpControlMsg(const MsgPtr &aReply, const Callback &aCallback): + reply(aReply), cbSuccess(aCallback) {} + +public: + MsgPtr reply; ///< the 1xx message being forwarded + Callback cbSuccess; ///< called after successfully writing the 1xx message + + // We could add an API to notify of send failures as well, but the + // current Source and Sink are tied via Store anyway, so the Source + // will know, eventually, if the Sink is gone or otherwise failed. +}; + +inline std::ostream & +operator <<(std::ostream &os, const HttpControlMsg &msg) +{ + return os << msg.reply << ", " << msg.cbSuccess; +} + +#endif /* SQUID_HTTP_CONTROL_MSG_H */ === modified file 'src/HttpMsg.h' --- src/HttpMsg.h 2010-01-01 21:16:57 +0000 +++ src/HttpMsg.h 2010-08-19 04:48:27 +0000 @@ -159,4 +159,41 @@ #define HTTPMSGUNLOCK(a) if(a){(a)->_unlock();(a)=NULL;} #define HTTPMSGLOCK(a) (a)->_lock() +// TODO: replace HTTPMSGLOCK with general RefCounting and delete this class +/// safe HttpMsg pointer wrapper that locks and unlocks the message +template +class HttpMsgPointerT +{ +public: + HttpMsgPointerT(): msg(NULL) {} + explicit HttpMsgPointerT(Msg *m): msg(m) { lock(); } + virtual ~HttpMsgPointerT() { unlock(); } + + HttpMsgPointerT(const HttpMsgPointerT &p): msg(p.msg) { lock(); } + HttpMsgPointerT &operator =(const HttpMsgPointerT &p) + { if (msg != p.msg) { unlock(); msg = p.msg; lock(); } return *this; } + + Msg &operator *() { return *msg; } + const Msg &operator *() const { return *msg; } + Msg *operator ->() { return msg; } + const Msg *operator ->() const { return msg; } + operator Msg *() { return msg; } + operator const Msg *() const { return msg; } + // add more as needed + + void lock() { if (msg) HTTPMSGLOCK(msg); } ///< prevent msg destruction + void unlock() { HTTPMSGUNLOCK(msg); } ///< allows/causes msg destruction + +private: + Msg *msg; +}; + +/// convenience wrapper to create HttpMsgPointerT<> object based on msg type +template +inline +HttpMsgPointerT HttpMsgPointer(Msg *msg) +{ + return HttpMsgPointerT(msg); +} + #endif /* SQUID_HTTPMSG_H */ === modified file 'src/HttpRequest.cc' --- src/HttpRequest.cc 2010-07-13 16:43:00 +0000 +++ src/HttpRequest.cc 2010-08-17 20:59:49 +0000 @@ -644,3 +644,16 @@ return rangeOffsetLimit; } + +bool +HttpRequest::canHandle1xx() const +{ + // old clients do not support 1xx by default + if (http_ver <= HttpVersion(1,0) && !header.has(HDR_EXPECT)) + return false; + + // TODO: add configurable exceptions here + + // others must support 1xx + return true; +} === modified file 'src/HttpRequest.h' --- src/HttpRequest.h 2010-04-17 10:38:50 +0000 +++ src/HttpRequest.h 2010-08-18 16:17:43 +0000 @@ -88,6 +88,9 @@ /* are responses to this request potentially cachable */ bool cacheable() const; + /// whether the client is likely to be able to handle a 1xx reply + bool canHandle1xx() const; + /* Now that we care what host contains it is better off being protected. */ /* HACK: These two methods are only inline to get around Makefile dependancies */ /* caused by HttpRequest being used in places it really shouldn't. */ @@ -248,6 +251,9 @@ cbdataReferenceDone(pinned_connection); } + // XXX: needs cbdataReference protection! + ConnStateData *clientConnection; ///< client-side conn manager, if known + int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */ private: === modified file 'src/HttpVersion.h' --- src/HttpVersion.h 2009-01-21 03:47:47 +0000 +++ src/HttpVersion.h 2010-08-17 17:47:55 +0000 @@ -66,6 +66,23 @@ return ((this->major != that.major) || (this->minor != that.minor)); } + bool operator <(const HttpVersion& that) const { + return (this->major < that.major || + this->major == that.major && this->minor < that.minor); + } + + bool operator >(const HttpVersion& that) const { + return (this->major > that.major || + this->major == that.major && this->minor > that.minor); + } + + bool operator <=(const HttpVersion& that) const { + return !(*this > that); + } + + bool operator >=(const HttpVersion& that) const { + return !(*this < that); + } }; #endif /* SQUID_HTTPVERSION_H */ === modified file 'src/Makefile.am' --- src/Makefile.am 2010-08-16 21:20:53 +0000 +++ src/Makefile.am 2010-08-19 04:47:30 +0000 @@ -354,6 +354,7 @@ HttpBody.cc \ HttpMsg.cc \ HttpMsg.h \ + HttpControlMsg.h \ HttpReply.cc \ HttpReply.h \ HttpRequest.cc \ === modified file 'src/cf.data.pre' --- src/cf.data.pre 2010-08-14 16:38:27 +0000 +++ src/cf.data.pre 2010-08-19 05:37:57 +0000 @@ -4164,21 +4164,6 @@ or response to be rejected. DOC_END -NAME: ignore_expect_100 -COMMENT: on|off -IFDEF: USE_HTTP_VIOLATIONS -TYPE: onoff -LOC: Config.onoff.ignore_expect_100 -DEFAULT: off -DOC_START - This option makes Squid ignore any Expect: 100-continue header present - in the request. RFC 2616 requires that Squid being unable to satisfy - the response expectation MUST return a 417 error. - - Note: Enabling this is a HTTP protocol violation, but some clients may - not handle it well.. -DOC_END - COMMENT_START TIMEOUTS ----------------------------------------------------------------------------- === modified file 'src/client_side.cc' --- src/client_side.cc 2010-08-07 14:22:54 +0000 +++ src/client_side.cc 2010-08-19 05:32:19 +0000 @@ -342,6 +342,59 @@ return newContext; } +void +ClientSocketContext::writeControlMsg(HttpControlMsg &msg) +{ + HttpReply *rep = msg.reply; + Must(rep); + + // apply selected clientReplyContext::buildReplyHeader() mods + // it is not clear what headers are required for control messages + rep->header.removeHopByHopEntries(); + rep->header.putStr(HDR_CONNECTION, "keep-alive"); + httpHdrMangleList(&rep->header, http->request, ROR_REPLY); + + // remember the callback + cbControlMsgSent = msg.cbSuccess; + + MemBuf *mb = rep->pack(); + + AsyncCall::Pointer call = commCbCall(33, 5, "ClientSocketContext::wroteControlMsg", + CommIoCbPtrFun(&WroteControlMsg, this)); + comm_write_mbuf(fd(), mb, call); + + delete mb; +} + +/// called when we wrote the 1xx response +void +ClientSocketContext::wroteControlMsg(int fd, char *, size_t, comm_err_t errflag, int xerrno) +{ + if (errflag == COMM_ERR_CLOSING) + return; + + if (errflag == COMM_OK) { + ScheduleCallHere(cbControlMsgSent); + return; + } + + debugs(33, 3, HERE << "1xx writing failed: " << xstrerr(xerrno)); + // no error notification: see HttpControlMsg.h for rationale and + // note that some errors are detected elsewhere (e.g., close handler) + + // close on 1xx errors to be conservative and to simplify the code + // (if we do not close, we must notify the source of a failure!) + comm_close(fd); +} + +/// wroteControlMsg() wrapper: ClientSocketContext is not an AsyncJob +void +ClientSocketContext::WroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno, void *data) +{ + ClientSocketContext *context = static_cast(data); + context->wroteControlMsg(fd, bufnotused, size, errflag, xerrno); +} + #if USE_IDENT static void clientIdentDone(const char *ident, void *data) @@ -742,10 +795,7 @@ debugs(33, 3, "clientSetKeepaliveFlag: method = " << RequestMethodStr(request->method)); - /* We are HTTP/1.1 facing clients now*/ - HttpVersion http_ver(1,1); - - if (httpMsgIsPersistent(http_ver, req_hdr)) + if (httpMsgIsPersistent(request->http_ver, req_hdr)) request->flags.proxy_keepalive = 1; } @@ -2527,16 +2577,9 @@ } if (request->header.has(HDR_EXPECT)) { - int ignore = 0; -#if USE_HTTP_VIOLATIONS - if (Config.onoff.ignore_expect_100) { - String expect = request->header.getList(HDR_EXPECT); - if (expect.caseCmp("100-continue") == 0) - ignore = 1; - expect.clean(); - } -#endif - if (!ignore) { + const String expect = request->header.getList(HDR_EXPECT); + const bool supportedExpect = (expect.caseCmp("100-continue") == 0); + if (!supportedExpect) { clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); @@ -3859,6 +3902,24 @@ delete bodyParser; // TODO: pool } +void +ConnStateData::sendControlMsg(HttpControlMsg msg) +{ + ClientSocketContext::Pointer context = getCurrentContext(); + if (context != NULL) { + context->writeControlMsg(msg); // will call msg.cbSuccess + return; + } + + if (!isOpen()) { + debugs(33, 3, HERE << "ignoring 1xx due to earlier closure"); + return; + } + + debugs(33, 3, HERE << " closing due to missing context for 1xx"); + comm_close(fd); +} + /* This is a comm call normally scheduled by comm_close() */ void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) === modified file 'src/client_side.h' --- src/client_side.h 2010-07-30 20:11:20 +0000 +++ src/client_side.h 2010-08-19 04:59:25 +0000 @@ -42,6 +42,7 @@ #include "eui/Eui64.h" #include "RefCount.h" #include "StoreIOBuffer.h" +#include "HttpControlMsg.h" class ConnStateData; class ClientHttpRequest; @@ -112,6 +113,13 @@ void registerWithConn(); void noteIoError(const int xerrno); ///< update state to reflect I/O error + // starts writing 1xx control message to the client + void writeControlMsg(HttpControlMsg &msg); + +protected: + static void WroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno, void *data); + void wroteControlMsg(int fd, char *bufnotused, size_t size, comm_err_t errflag, int xerrno); + private: CBDATA_CLASS(ClientSocketContext); void prepareReply(HttpReply * rep); @@ -119,6 +127,9 @@ void deRegisterWithConn(); void doClose(); void initiateClose(const char *reason); + + AsyncCall::Pointer cbControlMsgSent; ///< notifies HttpControlMsg Source + bool mayUseConnection_; /* This request may use the connection. Don't read anymore requests for now */ bool connRegistered_; }; @@ -127,7 +138,7 @@ class ConnectionDetail; /** A connection to a socket */ -class ConnStateData : public BodyProducer/*, public RefCountable*/ +class ConnStateData : public BodyProducer, public HttpControlMsgSink { public: @@ -147,6 +158,9 @@ int getConcurrentRequestCount() const; bool isOpen() const; + // HttpControlMsgSink API + virtual void sendControlMsg(HttpControlMsg msg); + int fd; /// chunk buffering and parsing algorithm state === modified file 'src/client_side_reply.cc' --- src/client_side_reply.cc 2010-08-14 02:58:39 +0000 +++ src/client_side_reply.cc 2010-08-18 16:56:17 +0000 @@ -261,6 +261,8 @@ http->storeEntry(entry); assert(http->out.offset == 0); + http->request->clientConnection = http->getConn(); + /* * A refcounted pointer so that FwdState stays around as long as * this clientReplyContext does @@ -680,6 +682,8 @@ if (http->flags.internal) r->protocol = PROTO_INTERNAL; + r->clientConnection = http->getConn(); + /** Start forwarding to get the new object from network */ FwdState::fwdStart(http->getConn() != NULL ? http->getConn()->fd : -1, http->storeEntry(), === modified file 'src/http.cc' --- src/http.cc 2010-08-13 07:53:08 +0000 +++ src/http.cc 2010-08-19 15:38:15 +0000 @@ -54,6 +54,7 @@ #include "HttpHdrScTarget.h" #include "HttpReply.h" #include "HttpRequest.h" +#include "HttpControlMsg.h" #include "MemBuf.h" #include "MemObject.h" #include "protos.h" @@ -705,23 +706,9 @@ readBuf->consume(header_bytes_read); } - /* Skip 1xx messages for now. Advertised in Via as an internal 1.0 hop */ if (newrep->sline.protocol == PROTO_HTTP && newrep->sline.status >= 100 && newrep->sline.status < 200) { - -#if WHEN_HTTP11_EXPECT_HANDLED - /* When HTTP/1.1 check if the client is expecting a 1xx reply and maybe pass it on */ - if (orig_request->header.has(HDR_EXPECT)) { - // TODO: pass to the client anyway? - } -#endif - delete newrep; - debugs(11, 2, HERE << "1xx headers consume " << header_bytes_read << " bytes header."); - header_bytes_read = 0; - if (reply_bytes_read > 0) - debugs(11, 2, HERE << "1xx headers consume " << reply_bytes_read << " bytes reply."); - reply_bytes_read = 0; + handle1xx(newrep); ctx_exit(ctx); - processReplyHeader(); return; } @@ -752,6 +739,57 @@ ctx_exit(ctx); } +/// ignore or start forwarding the 1xx response (a.k.a., control message) +void +HttpStateData::handle1xx(HttpReply *reply) +{ + HttpMsgPointerT msg(reply); // will destroy reply if unused + + // one 1xx at a time: we must not be called while waiting for previous 1xx + Must(!flags.handling1xx); + flags.handling1xx = true; + + if (!orig_request->canHandle1xx()) { + debugs(11, 2, HERE << "ignoring client-unsupported 1xx"); + proceedAfter1xx(); + return; + } + + debugs(11, 2, HERE << "forwarding 1xx to client"); + + // the Sink will use this to call us back after writing 1xx to the client + typedef NullaryMemFunT CbDialer; + AsyncCall::Pointer cb = asyncCall(11, 3, "HttpStateData::proceedAfter1xx", + CbDialer(this, &HttpStateData::proceedAfter1xx)); + + typedef UnaryMemFunT SinkDialer; + ConnStateData *conn = orig_request->clientConnection; + AsyncCall::Pointer call = asyncCall(11, 4, "ConnStateData::sendControlMessage", + SinkDialer(conn, &ConnStateData::sendControlMsg, HttpControlMsg(msg, cb))); + ScheduleCallHere(call); + // If the call is not fired, then the Sink is gone, and HttpStateData + // will terminate due to an aborted store entry or another similar error. + // If we get stuck, it is not handle1xx fault, because we could get stuck + // for similar reasons without a 1xx response! +} + +/// restores state and resumes processing after 1xx is ignored or forwarded +void +HttpStateData::proceedAfter1xx() +{ + Must(flags.handling1xx); + + debugs(11, 2, HERE << "consuming " << header_bytes_read << + " header and " << reply_bytes_read << " body bytes read after 1xx"); + header_bytes_read = 0; + reply_bytes_read = 0; + + AsyncCall::Pointer call = asyncCall(11, 3, "HttpStateData::processReply", + MemFun(this, &HttpStateData::processReply)); + ScheduleCallHere(call); +} + + /** * returns true if the peer can support connection pinning */ @@ -1132,6 +1170,20 @@ } } + processReply(); +} + +/// processes the already read and buffered response data, possibly after +/// waiting for asynchronous 1xx control message processing +void +HttpStateData::processReply() { + + if (flags.handling1xx) { // we came back after handling a 1xx response + debugs(11, 5, HERE << "done with 1xx handling"); + flags.handling1xx = false; + Must(!flags.headers_parsed); + } + if (!flags.headers_parsed) { // have not parsed headers yet? PROF_start(HttpStateData_processReplyHeader); processReplyHeader(); @@ -1155,6 +1207,12 @@ bool HttpStateData::continueAfterParsingHeader() { + if (flags.handling1xx) { + debugs(11, 5, HERE << "wait for 1xx handling"); + Must(!flags.headers_parsed); + return false; + } + if (!flags.headers_parsed && !eof) { debugs(11, 9, HERE << "needs more at " << readBuf->contentSize()); flags.do_next_read = 1; === modified file 'src/http.h' --- src/http.h 2010-07-28 18:04:45 +0000 +++ src/http.h 2010-08-19 05:11:26 +0000 @@ -81,6 +81,10 @@ protected: virtual HttpRequest *originalRequest(); + void processReply(); + void proceedAfter1xx(); + void handle1xx(HttpReply *msg); + private: AsyncCall::Pointer closeHandler; enum ConnectionStatus { === modified file 'src/structs.h' --- src/structs.h 2010-08-07 14:22:54 +0000 +++ src/structs.h 2010-08-17 21:16:01 +0000 @@ -760,6 +760,7 @@ unsigned int proxying:1; unsigned int keepalive:1; unsigned int only_if_cached:1; + unsigned int handling1xx:1; ///< we are ignoring or forwarding 1xx response unsigned int headers_parsed:1; unsigned int front_end_https:2; unsigned int originpeer:1;