If no objection I will apply this patch to trunk.
On 11/16/2016 02:35 PM, Amos Jeffries wrote:
On 16/11/2016 12:58 a.m., Christos Tsantilas wrote:
Hi all,
I applied the patch as r14945 with an r14946 fix.
Unfortunately while I was testing the Squid-3.5 variant of the patch I
found a bug:
When the Http::One::Server::writeControlMsgAndCall fails to write the
control message, schedules a Comm::Write callback using just a
ScheduleCallHere command.
The callback called without the CommIoCbParams details and squid is
crashes.
There are two ways to fix this issue.
1) complete the missing CommIoCbParams details inside
writeControlMsgAndCall
2) Make the ConnStateData::writeControlMsgAndCall to return false on
failures and allow caller handle the failure.
This patch implements the (2) as I believe it is the better option.
My apologies for the buggy patch.
Now that the method wroteControlMsgOK() is needing to be called in
not-OK situations I think it should have a different name.
Perhapse doneWithControlMsg() ?
Fix r14945: Fixed Write.cc:41 "!ccb->active()" assertion.
The r14945 patch has a major bug:
When the Http::One::Server::writeControlMsgAndCall fails to write the control
message, schedules a Comm::Write callback using just a ScheduleCallHere command.
The callback called withtout the CommIoCbParams details and squid is crashes.
This patch fixes the ConnStateData::writeControlMsgAndCall to return false if it
fails to write the control message and allow the caller to handle the failure.
This is a Measurement Factory project
=== modified file 'src/HttpControlMsg.cc'
--- src/HttpControlMsg.cc 2016-11-15 09:41:26 +0000
+++ src/HttpControlMsg.cc 2016-11-16 14:36:38 +0000
@@ -1,46 +1,46 @@
/*
* Copyright (C) 1996-2016 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/
#include "squid.h"
#include "comm/Flag.h"
#include "CommCalls.h"
#include "HttpControlMsg.h"
void
-HttpControlMsgSink::wroteControlMsgOK()
+HttpControlMsgSink::doneWithControlMsg()
{
if (cbControlMsgSent) {
ScheduleCallHere(cbControlMsgSent);
cbControlMsgSent = nullptr;
}
}
/// called when we wrote the 1xx response
void
HttpControlMsgSink::wroteControlMsg(const CommIoCbParams ¶ms)
{
if (params.flag == Comm::ERR_CLOSING)
return;
if (params.flag == Comm::OK) {
- wroteControlMsgOK();
+ doneWithControlMsg();
return;
}
debugs(33, 3, "1xx writing failed: " << xstrerr(params.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!)
params.conn->close();
// XXX: writeControlMsgAndCall() should handle writer-specific writing
// results, including errors and then call us with success/failure outcome.
}
=== modified file 'src/HttpControlMsg.h'
--- src/HttpControlMsg.h 2016-11-15 09:41:26 +0000
+++ src/HttpControlMsg.h 2016-11-16 14:32:22 +0000
@@ -16,41 +16,41 @@
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;
- virtual void wroteControlMsgOK();
+ virtual void doneWithControlMsg();
/// callback to handle Comm::Write completion
void wroteControlMsg(const CommIoCbParams &);
/// Call to schedule when the control msg has been sent
AsyncCall::Pointer cbControlMsgSent;
};
/// bundles HTTP 1xx reply and the "successfully forwarded" callback
class HttpControlMsg
{
public:
typedef AsyncCall::Pointer Callback;
HttpControlMsg(const HttpReply::Pointer &aReply, const Callback &aCallback):
reply(aReply), cbSuccess(aCallback) {}
public:
HttpReply::Pointer reply; ///< the 1xx message being forwarded
Callback cbSuccess; ///< called after successfully writing the 1xx message
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2016-11-15 09:41:26 +0000
+++ src/client_side.cc 2016-11-16 14:33:45 +0000
@@ -3808,52 +3808,55 @@
// XXX: this is an HTTP/1-only operation
void
ConnStateData::sendControlMsg(HttpControlMsg msg)
{
if (!isOpen()) {
debugs(33, 3, HERE << "ignoring 1xx due to earlier closure");
return;
}
// HTTP/1 1xx status messages are only valid when there is a transaction to trigger them
if (!pipeline.empty()) {
HttpReply::Pointer rep(msg.reply);
Must(rep);
// remember the callback
cbControlMsgSent = msg.cbSuccess;
typedef CommCbMemFunT<HttpControlMsgSink, CommIoCbParams> Dialer;
AsyncCall::Pointer call = JobCallback(33, 5, Dialer, this, HttpControlMsgSink::wroteControlMsg);
- writeControlMsgAndCall(rep.getRaw(), call);
+ if (!writeControlMsgAndCall(rep.getRaw(), call)) {
+ // but still inform the caller (so it may resume its operation)
+ doneWithControlMsg();
+ }
return;
}
debugs(33, 3, HERE << " closing due to missing context for 1xx");
clientConnection->close();
}
void
-ConnStateData::wroteControlMsgOK()
+ConnStateData::doneWithControlMsg()
{
- HttpControlMsgSink::wroteControlMsgOK();
+ HttpControlMsgSink::doneWithControlMsg();
if (Http::StreamPointer deferredRequest = pipeline.front()) {
debugs(33, 3, clientConnection << ": calling PushDeferredIfNeeded after control msg wrote");
ClientSocketContextPushDeferredIfNeeded(deferredRequest, this);
}
}
/// Our close handler called by Comm when the pinned connection is closed
void
ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io)
{
// FwdState might repin a failed connection sooner than this close
// callback is called for the failed connection.
assert(pinning.serverConnection == io.conn);
pinning.closeHandler = NULL; // Comm unregisters handlers before calling
const bool sawZeroReply = pinning.zeroReply; // reset when unpinning
pinning.serverConnection->noteClosure();
unpinConnection(false);
if (sawZeroReply && clientConnection != NULL) {
=== modified file 'src/client_side.h'
--- src/client_side.h 2016-11-15 09:41:26 +0000
+++ src/client_side.h 2016-11-16 14:34:10 +0000
@@ -60,41 +60,41 @@
*
* To terminate a ConnStateData close() the client Comm::Connection it is
* managing, or for graceful half-close use the stopReceiving() or
* stopSending() methods.
*/
class ConnStateData : public Server, public HttpControlMsgSink, private IndependentRunner
{
public:
explicit ConnStateData(const MasterXaction::Pointer &xact);
virtual ~ConnStateData();
/* ::Server API */
virtual void receivedFirstByte();
virtual bool handleReadData();
virtual void afterClientRead();
virtual void afterClientWrite(size_t);
/* HttpControlMsgSink API */
virtual void sendControlMsg(HttpControlMsg);
- virtual void wroteControlMsgOK();
+ virtual void doneWithControlMsg();
/// Traffic parsing
bool clientParseRequests();
void readNextRequest();
/// try to make progress on a transaction or read more I/O
void kick();
bool isOpen() const;
Http1::TeChunkedParser *bodyParser; ///< parses HTTP/1.1 chunked request body
/** number of body bytes we need to comm_read for the "current" request
*
* \retval 0 We do not need to read any [more] body bytes
* \retval negative May need more but do not know how many; could be zero!
* \retval positive Need to read exactly that many more body bytes
*/
int64_t mayNeedToReadMoreBody() const;
@@ -247,41 +247,41 @@
/// Called when the client sends the first request on a bumped connection.
/// Returns false if no [delayed] error should be written to the client.
/// Otherwise, writes the error to the client and returns true. Also checks
/// for SQUID_X509_V_ERR_DOMAIN_MISMATCH on bumped requests.
bool serveDelayedError(Http::Stream *);
Ssl::BumpMode sslBumpMode; ///< ssl_bump decision (Ssl::bumpEnd if n/a).
/// Tls parser to use for client HELLO messages parsing on bumped
/// connections.
Security::HandshakeParser tlsParser;
#else
bool switchedToHttps() const { return false; }
#endif
/* clt_conn_tag=tag annotation access */
const SBuf &connectionTag() const { return connectionTag_; }
void connectionTag(const char *aTag) { connectionTag_ = aTag; }
/// handle a control message received by context from a peer and call back
- virtual void writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) = 0;
+ virtual bool writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) = 0;
/// ClientStream calls this to supply response header (once) and data
/// for the current Http::Stream.
virtual void handleReply(HttpReply *header, StoreIOBuffer receivedData) = 0;
/// remove no longer needed leading bytes from the input buffer
void consumeInput(const size_t byteCount);
/* TODO: Make the methods below (at least) non-public when possible. */
/// stop parsing the request and create context for relaying error info
Http::Stream *abortRequestParsing(const char *const errUri);
/// generate a fake CONNECT request with the given payload
/// at the beginning of the client I/O buffer
bool fakeAConnectRequest(const char *reason, const SBuf &payload);
/// generates and sends to tunnel.cc a fake request with a given payload
bool initiateTunneledRequest(HttpRequest::Pointer const &cause, Http::MethodType const method, const char *reason, const SBuf &payload);
=== modified file 'src/servers/FtpServer.cc'
--- src/servers/FtpServer.cc 2016-11-04 16:47:34 +0000
+++ src/servers/FtpServer.cc 2016-11-15 11:23:51 +0000
@@ -1136,46 +1136,47 @@
mb.appendf("%i %s\r\n", scode, reason); // error terminating line
// TODO: errorpage.cc should detect FTP client and use
// configurable FTP-friendly error templates which we should
// write to the client "as is" instead of hiding most of the info
writeReply(mb);
}
/// writes FTP response based on HTTP reply that is not an FTP-response wrapper
/// for example, internally-generated Squid "errorpages" end up here (for now)
void
Ftp::Server::writeForwardedForeign(const HttpReply *reply)
{
changeState(fssConnected, "foreign reply");
closeDataConnection();
// 451: We intend to keep the control connection open.
writeErrorReply(reply, 451);
}
-void
+bool
Ftp::Server::writeControlMsgAndCall(HttpReply *reply, AsyncCall::Pointer &call)
{
// the caller guarantees that we are dealing with the current context only
// the caller should also make sure reply->header.has(Http::HdrType::FTP_STATUS)
writeForwardedReplyAndCall(reply, call);
+ return true;
}
void
Ftp::Server::writeForwardedReplyAndCall(const HttpReply *reply, AsyncCall::Pointer &call)
{
assert(reply != NULL);
const HttpHeader &header = reply->header;
// without status, the caller must use the writeForwardedForeign() path
Must(header.has(Http::HdrType::FTP_STATUS));
Must(header.has(Http::HdrType::FTP_REASON));
const int scode = header.getInt(Http::HdrType::FTP_STATUS);
debugs(33, 7, "scode: " << scode);
// Status 125 or 150 implies upload or data request, but we still check
// the state in case the server is buggy.
if ((scode == 125 || scode == 150) &&
(master->serverState == fssHandleUploadRequest ||
master->serverState == fssHandleDataRequest)) {
if (checkDataConnPost()) {
=== modified file 'src/servers/FtpServer.h'
--- src/servers/FtpServer.h 2016-11-04 16:47:34 +0000
+++ src/servers/FtpServer.h 2016-11-15 11:17:31 +0000
@@ -80,41 +80,41 @@
friend void StartListening();
// errors detected before it is possible to create an HTTP request wrapper
enum class EarlyErrorKind {
HugeRequest,
MissingLogin,
MissingUsername,
MissingHost,
UnsupportedCommand,
InvalidUri,
MalformedCommand
};
/* ConnStateData API */
virtual Http::Stream *parseOneRequest() override;
virtual void processParsedRequest(Http::StreamPointer &context) override;
virtual void notePeerConnection(Comm::ConnectionPointer conn) override;
virtual void clientPinnedConnectionClosed(const CommCloseCbParams &io) override;
virtual void handleReply(HttpReply *header, StoreIOBuffer receivedData) override;
virtual int pipelinePrefetchMax() const override;
- virtual void writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) override;
+ virtual bool writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) override;
virtual time_t idleTimeout() const override;
/* BodyPipe API */
virtual void noteMoreBodySpaceAvailable(BodyPipe::Pointer) override;
virtual void noteBodyConsumerAborted(BodyPipe::Pointer ptr) override;
/* AsyncJob API */
virtual void start() override;
/* Comm callbacks */
static void AcceptCtrlConnection(const CommAcceptCbParams ¶ms);
void acceptDataConnection(const CommAcceptCbParams ¶ms);
void readUploadData(const CommIoCbParams &io);
void wroteEarlyReply(const CommIoCbParams &io);
void wroteReply(const CommIoCbParams &io);
void wroteReplyData(const CommIoCbParams &io);
void connectedForData(const CommConnectCbParams ¶ms);
unsigned int listenForDataConnection();
bool createDataConnection(Ip::Address cltAddr);
=== modified file 'src/servers/Http1Server.cc'
--- src/servers/Http1Server.cc 2016-11-15 09:41:26 +0000
+++ src/servers/Http1Server.cc 2016-11-15 11:23:09 +0000
@@ -289,65 +289,64 @@
!context->startOfOutput();
const bool responseFinishedOrFailed = !rep &&
!receivedData.data &&
!receivedData.length;
if (responseFinishedOrFailed && !mustSendLastChunk) {
context->writeComplete(0);
return;
}
if (!context->startOfOutput()) {
context->sendBody(receivedData);
return;
}
assert(rep);
http->al->reply = rep;
HTTPMSGLOCK(http->al->reply);
context->sendStartOfMessage(rep, receivedData);
}
-void
+bool
Http::One::Server::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call)
{
Http::StreamPointer context = pipeline.front();
Must(context != nullptr);
// Ignore this late control message if we have started sending a
// reply to the user already (e.g., after an error).
if (context->reply) {
debugs(11, 2, "drop 1xx made late by " << context->reply);
- // but still inform the caller (so it may resume its operation)
- ScheduleCallHere(call);
- return;
+ return false;
}
const ClientHttpRequest *http = context->http;
// apply selected clientReplyContext::buildReplyHeader() mods
// it is not clear what headers are required for control messages
rep->header.removeHopByHopEntries();
rep->header.putStr(Http::HdrType::CONNECTION, "keep-alive");
httpHdrMangleList(&rep->header, http->request, http->al, ROR_REPLY);
MemBuf *mb = rep->pack();
debugs(11, 2, "HTTP Client " << clientConnection);
debugs(11, 2, "HTTP Client CONTROL MSG:\n---------\n" << mb->buf << "\n----------");
Comm::Write(clientConnection, mb, call);
delete mb;
+ return true;
}
ConnStateData *
Http::NewServer(MasterXactionPointer &xact)
{
return new Http1::Server(xact, false);
}
ConnStateData *
Https::NewServer(MasterXactionPointer &xact)
{
return new Http1::Server(xact, true);
}
=== modified file 'src/servers/Http1Server.h'
--- src/servers/Http1Server.h 2016-11-04 16:47:34 +0000
+++ src/servers/Http1Server.h 2016-11-15 11:14:28 +0000
@@ -15,41 +15,41 @@
{
namespace One
{
/// Manages a connection from an HTTP/1 or HTTP/0.9 client.
class Server: public ConnStateData
{
CBDATA_CLASS(Server);
public:
Server(const MasterXaction::Pointer &xact, const bool beHttpsServer);
virtual ~Server() {}
void readSomeHttpData();
protected:
/* ConnStateData API */
virtual Http::Stream *parseOneRequest();
virtual void processParsedRequest(Http::StreamPointer &context);
virtual void handleReply(HttpReply *rep, StoreIOBuffer receivedData);
- virtual void writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call);
+ virtual bool writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call);
virtual time_t idleTimeout() const;
/* BodyPipe API */
virtual void noteMoreBodySpaceAvailable(BodyPipe::Pointer);
virtual void noteBodyConsumerAborted(BodyPipe::Pointer);
/* AsyncJob API */
virtual void start();
void proceedAfterBodyContinuation(Http::StreamPointer context);
private:
void processHttpRequest(Http::Stream *const context);
void handleHttpRequestData();
/// Handles parsing results. May generate and deliver an error reply
/// to the client if parsing is failed, or parses the url and build the
/// HttpRequest object using parsing results.
/// Return false if parsing is failed, true otherwise.
bool buildHttpRequest(Http::StreamPointer &context);
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
