From 2f97ab10f665ecfc06323f0b5cb5fcd7d0f1a103 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 17 Nov 2016 12:13:41 +0200 Subject: [PATCH] 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 --- src/HttpControlMsg.cc | 4 ++-- src/HttpControlMsg.h | 2 +- src/client_side.cc | 9 ++++++--- src/client_side.h | 4 ++-- src/servers/FtpServer.cc | 3 ++- src/servers/FtpServer.h | 2 +- src/servers/Http1Server.cc | 7 +++---- src/servers/Http1Server.h | 2 +- src/tests/stub_HttpControlMsg.cc | 2 +- 9 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/HttpControlMsg.cc b/src/HttpControlMsg.cc index 1efc69b7cc..29f9888a7e 100644 --- a/src/HttpControlMsg.cc +++ b/src/HttpControlMsg.cc @@ -12,7 +12,7 @@ #include "HttpControlMsg.h" void -HttpControlMsgSink::wroteControlMsgOK() +HttpControlMsgSink::doneWithControlMsg() { if (cbControlMsgSent) { ScheduleCallHere(cbControlMsgSent); @@ -28,7 +28,7 @@ HttpControlMsgSink::wroteControlMsg(const CommIoCbParams ¶ms) return; if (params.flag == Comm::OK) { - wroteControlMsgOK(); + doneWithControlMsg(); return; } diff --git a/src/HttpControlMsg.h b/src/HttpControlMsg.h index eb51dd3e04..cfeb53903b 100644 --- a/src/HttpControlMsg.h +++ b/src/HttpControlMsg.h @@ -33,7 +33,7 @@ public: /// 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 &); diff --git a/src/client_side.cc b/src/client_side.cc index ce37226459..63803a1c06 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3825,7 +3825,10 @@ ConnStateData::sendControlMsg(HttpControlMsg msg) typedef CommCbMemFunT 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; } @@ -3834,9 +3837,9 @@ ConnStateData::sendControlMsg(HttpControlMsg msg) } 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"); diff --git a/src/client_side.h b/src/client_side.h index 7d32b16596..ba0d328e71 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -77,7 +77,7 @@ public: /* HttpControlMsgSink API */ virtual void sendControlMsg(HttpControlMsg); - virtual void wroteControlMsgOK(); + virtual void doneWithControlMsg(); /// Traffic parsing bool clientParseRequests(); @@ -264,7 +264,7 @@ public: 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. diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index c30a1adb7a..42cbd0147d 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1153,12 +1153,13 @@ Ftp::Server::writeForwardedForeign(const HttpReply *reply) 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 diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h index f3025ba411..e44f7d5d39 100644 --- a/src/servers/FtpServer.h +++ b/src/servers/FtpServer.h @@ -97,7 +97,7 @@ protected: 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 */ diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc index fd5ef7796f..4129e0d4ce 100644 --- a/src/servers/Http1Server.cc +++ b/src/servers/Http1Server.cc @@ -306,7 +306,7 @@ Http::One::Server::handleReply(HttpReply *rep, StoreIOBuffer receivedData) context->sendStartOfMessage(rep, receivedData); } -void +bool Http::One::Server::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &call) { Http::StreamPointer context = pipeline.front(); @@ -316,9 +316,7 @@ Http::One::Server::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &ca // 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; @@ -337,6 +335,7 @@ Http::One::Server::writeControlMsgAndCall(HttpReply *rep, AsyncCall::Pointer &ca Comm::Write(clientConnection, mb, call); delete mb; + return true; } ConnStateData * diff --git a/src/servers/Http1Server.h b/src/servers/Http1Server.h index 4187a5c47d..d1cd50f865 100644 --- a/src/servers/Http1Server.h +++ b/src/servers/Http1Server.h @@ -32,7 +32,7 @@ protected: 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 */ diff --git a/src/tests/stub_HttpControlMsg.cc b/src/tests/stub_HttpControlMsg.cc index ae01a9d138..9fa482dc06 100644 --- a/src/tests/stub_HttpControlMsg.cc +++ b/src/tests/stub_HttpControlMsg.cc @@ -13,5 +13,5 @@ #include "HttpControlMsg.h" void HttpControlMsgSink::wroteControlMsg(CommIoCbParams const&) STUB -void HttpControlMsgSink::wroteControlMsgOK() STUB +void HttpControlMsgSink::doneWithControlMsg() STUB -- 2.47.2