From: Gunnar Beutner Date: Tue, 29 Sep 2015 08:31:16 +0000 (+0200) Subject: Fix deadlock in TlsStream::Close X-Git-Tag: v2.4.0~258 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0e40c3ee1d1a40604f18c9be650370ca55286437;hp=8dec9538293bbf0ef58ca1673a002155f73af631;p=icinga2 Fix deadlock in TlsStream::Close fixes #10235 --- diff --git a/icinga-studio/api.cpp b/icinga-studio/api.cpp index a92ecabb0..2fd0d7803 100644 --- a/icinga-studio/api.cpp +++ b/icinga-studio/api.cpp @@ -54,33 +54,32 @@ void ApiClient::TypesHttpCompletionCallback(HttpRequest& request, HttpResponse& while ((count = response.ReadBody(buffer, sizeof(buffer))) > 0) body += String(buffer, buffer + count); + std::vector types; + if (response.StatusCode < 200 || response.StatusCode > 299) { Log(LogCritical, "ApiClient") << "Failed HTTP request; Code: " << response.StatusCode << "; Body: " << body; - return; - } + } else { + try { + result = JsonDecode(body); - std::vector types; + Array::Ptr results = result->Get("results"); - try { - result = JsonDecode(body); - - Array::Ptr results = result->Get("results"); - - ObjectLock olock(results); - BOOST_FOREACH(const Dictionary::Ptr typeInfo, results) - { - ApiType::Ptr type = new ApiType();; - type->Abstract = typeInfo->Get("abstract"); - type->BaseName = typeInfo->Get("base"); - type->Name = typeInfo->Get("name"); - type->PluralName = typeInfo->Get("plural_name"); - // TODO: attributes - types.push_back(type); + ObjectLock olock(results); + BOOST_FOREACH(const Dictionary::Ptr typeInfo, results) + { + ApiType::Ptr type = new ApiType();; + type->Abstract = typeInfo->Get("abstract"); + type->BaseName = typeInfo->Get("base"); + type->Name = typeInfo->Get("name"); + type->PluralName = typeInfo->Get("plural_name"); + // TODO: attributes + types.push_back(type); + } + } catch (const std::exception& ex) { + Log(LogCritical, "ApiClient") + << "Error while decoding response: " << DiagnosticInformation(ex); } - } catch (const std::exception& ex) { - Log(LogCritical, "ApiClient") - << "Error while decoding response: " << DiagnosticInformation(ex); } callback(types); @@ -125,54 +124,54 @@ void ApiClient::ObjectsHttpCompletionCallback(HttpRequest& request, while ((count = response.ReadBody(buffer, sizeof(buffer))) > 0) body += String(buffer, buffer + count); + std::vector objects; + if (response.StatusCode < 200 || response.StatusCode > 299) { Log(LogCritical, "ApiClient") << "Failed HTTP request; Code: " << response.StatusCode << "; Body: " << body; return; - } + } else { + try { + result = JsonDecode(body); - std::vector objects; - - try { - result = JsonDecode(body); - - Array::Ptr results = result->Get("results"); + Array::Ptr results = result->Get("results"); - if (results) { - ObjectLock olock(results); - BOOST_FOREACH(const Dictionary::Ptr objectInfo, results) - { - ApiObject::Ptr object = new ApiObject(); + if (results) { + ObjectLock olock(results); + BOOST_FOREACH(const Dictionary::Ptr objectInfo, results) + { + ApiObject::Ptr object = new ApiObject(); - Dictionary::Ptr attrs = objectInfo->Get("attrs"); + Dictionary::Ptr attrs = objectInfo->Get("attrs"); - { - ObjectLock olock(attrs); - BOOST_FOREACH(const Dictionary::Pair& kv, attrs) { - object->Attrs[kv.first] = kv.second; + ObjectLock olock(attrs); + BOOST_FOREACH(const Dictionary::Pair& kv, attrs) + { + object->Attrs[kv.first] = kv.second; + } } - } - Array::Ptr used_by = objectInfo->Get("used_by"); + Array::Ptr used_by = objectInfo->Get("used_by"); - { - ObjectLock olock(used_by); - BOOST_FOREACH(const Dictionary::Ptr& refInfo, used_by) { - ApiObjectReference ref; - ref.Name = refInfo->Get("name"); - ref.Type = refInfo->Get("type"); - object->UsedBy.push_back(ref); + ObjectLock olock(used_by); + BOOST_FOREACH(const Dictionary::Ptr& refInfo, used_by) + { + ApiObjectReference ref; + ref.Name = refInfo->Get("name"); + ref.Type = refInfo->Get("type"); + object->UsedBy.push_back(ref); + } } - } - objects.push_back(object); + objects.push_back(object); + } } + } catch (const std::exception& ex) { + Log(LogCritical, "ApiClient") + << "Error while decoding response: " << DiagnosticInformation(ex); } - } catch (const std::exception& ex) { - Log(LogCritical, "ApiClient") - << "Error while decoding response: " << DiagnosticInformation(ex); } callback(objects); diff --git a/lib/base/socketevents.cpp b/lib/base/socketevents.cpp index 8c420e709..b86d9a5c7 100644 --- a/lib/base/socketevents.cpp +++ b/lib/base/socketevents.cpp @@ -166,10 +166,12 @@ void SocketEvents::WakeUpThread(bool wait) l_SocketIOFDChanged = true; - (void) send(l_SocketIOEventFDs[1], "T", 1, 0); + while (l_SocketIOFDChanged) { + (void) send(l_SocketIOEventFDs[1], "T", 1, 0); - while (l_SocketIOFDChanged) - l_SocketIOCV.wait(lock); + boost::system_time const timeout = boost::get_system_time() + boost::posix_time::milliseconds(50); + l_SocketIOCV.timed_wait(lock, timeout); + } } } else { (void) send(l_SocketIOEventFDs[1], "T", 1, 0); diff --git a/lib/base/stream.cpp b/lib/base/stream.cpp index 524cd0782..cfcdf0bb0 100644 --- a/lib/base/stream.cpp +++ b/lib/base/stream.cpp @@ -22,7 +22,7 @@ using namespace icinga; -void Stream::RegisterDataHandler(const boost::function& handler) +void Stream::RegisterDataHandler(const boost::function& handler) { if (SupportsWaiting()) OnDataAvailable.connect(handler); @@ -52,7 +52,7 @@ size_t Stream::Peek(void *buffer, size_t count, bool allow_partial) void Stream::SignalDataAvailable(void) { - OnDataAvailable(); + OnDataAvailable(this); { boost::mutex::scoped_lock lock(m_Mutex); diff --git a/lib/base/stream.hpp b/lib/base/stream.hpp index 1131d6408..32ed4bb8d 100644 --- a/lib/base/stream.hpp +++ b/lib/base/stream.hpp @@ -131,7 +131,7 @@ public: virtual bool IsDataAvailable(void) const; - void RegisterDataHandler(const boost::function& handler); + void RegisterDataHandler(const boost::function& handler); StreamReadStatus ReadLine(String *line, StreamReadContext& context, bool may_wait = false); @@ -139,7 +139,7 @@ protected: void SignalDataAvailable(void); private: - boost::signals2::signal OnDataAvailable; + boost::signals2::signal OnDataAvailable; boost::mutex m_Mutex; boost::condition_variable m_CV; diff --git a/lib/base/tlsstream.cpp b/lib/base/tlsstream.cpp index 9bcb5c6eb..ba0333b25 100644 --- a/lib/base/tlsstream.cpp +++ b/lib/base/tlsstream.cpp @@ -184,10 +184,12 @@ void TlsStream::OnEvent(int revents) if (rc > 0) { m_CurrentAction = TlsActionNone; - if (m_SendQ->GetAvailableBytes() > 0) - ChangeEvents(POLLIN|POLLOUT); - else - ChangeEvents(POLLIN); + if (!m_Eof) { + if (m_SendQ->GetAvailableBytes() > 0) + ChangeEvents(POLLIN|POLLOUT); + else + ChangeEvents(POLLIN); + } lock.unlock(); diff --git a/lib/remote/httpclientconnection.cpp b/lib/remote/httpclientconnection.cpp index e6688df71..bcc834ded 100644 --- a/lib/remote/httpclientconnection.cpp +++ b/lib/remote/httpclientconnection.cpp @@ -49,6 +49,9 @@ void HttpClientConnection::Reconnect(void) m_Context.~StreamReadContext(); new (&m_Context) StreamReadContext(); + m_Requests.clear(); + m_CurrentResponse.reset(); + TcpSocket::Ptr socket = new TcpSocket(); socket->Connect(m_Host, m_Port); @@ -59,9 +62,9 @@ void HttpClientConnection::Reconnect(void) /* m_Stream = new NetworkStream(socket); -- does not currently work because the NetworkStream class doesn't support async I/O */ - m_Stream->RegisterDataHandler(boost::bind(&HttpClientConnection::DataAvailableHandler, this)); + m_Stream->RegisterDataHandler(boost::bind(&HttpClientConnection::DataAvailableHandler, this, _1)); if (m_Stream->IsDataAvailable()) - DataAvailableHandler(); + DataAvailableHandler(m_Stream); } Stream::Ptr HttpClientConnection::GetStream(void) const @@ -95,8 +98,10 @@ bool HttpClientConnection::ProcessMessage(void) { bool res; - if (m_Requests.empty()) + if (m_Requests.empty()) { + m_Stream->Close(); return false; + } const std::pair, HttpCompletionCallback>& currentRequest = *m_Requests.begin(); HttpRequest& request = *currentRequest.first.get(); @@ -129,19 +134,26 @@ bool HttpClientConnection::ProcessMessage(void) return res; } -void HttpClientConnection::DataAvailableHandler(void) +void HttpClientConnection::DataAvailableHandler(const Stream::Ptr& stream) { boost::mutex::scoped_lock lock(m_DataHandlerMutex); + ASSERT(stream == m_Stream); + try { while (ProcessMessage()) ; /* empty loop body */ } catch (const std::exception& ex) { Log(LogWarning, "HttpClientConnection") - << "Error while reading Http request: " << DiagnosticInformation(ex); + << "Error while reading Http response: " << DiagnosticInformation(ex); Disconnect(); } + + if (m_Context.Eof) { + Log(LogWarning, "HttpClientConnection", "Encountered unexpected EOF while reading Http response."); + m_Stream->Close(); + } } boost::shared_ptr HttpClientConnection::NewRequest(void) diff --git a/lib/remote/httpclientconnection.hpp b/lib/remote/httpclientconnection.hpp index 72fcfeef9..50bddbc40 100644 --- a/lib/remote/httpclientconnection.hpp +++ b/lib/remote/httpclientconnection.hpp @@ -68,7 +68,7 @@ private: void Reconnect(void); bool ProcessMessage(void); - void DataAvailableHandler(void); + void DataAvailableHandler(const Stream::Ptr& stream); void ProcessMessageAsync(HttpRequest& request); }; diff --git a/lib/remote/httpresponse.cpp b/lib/remote/httpresponse.cpp index f129f51c7..c5c9c722a 100644 --- a/lib/remote/httpresponse.cpp +++ b/lib/remote/httpresponse.cpp @@ -34,10 +34,14 @@ HttpResponse::HttpResponse(const Stream::Ptr& stream, const HttpRequest& request void HttpResponse::SetStatus(int code, const String& message) { - ASSERT(m_State == HttpResponseStart); ASSERT(code >= 100 && code <= 599); ASSERT(!message.IsEmpty()); + if (m_State != HttpResponseStart) { + Log(LogWarning, "HttpResponse", "Tried to set Http response status after headers had already been sent."); + return; + } + String status = "HTTP/"; if (m_Request.ProtocolVersion == HttpVersion10) @@ -54,7 +58,11 @@ void HttpResponse::SetStatus(int code, const String& message) void HttpResponse::AddHeader(const String& key, const String& value) { - ASSERT(m_State = HttpResponseHeaders); + if (m_State != HttpResponseHeaders) { + Log(LogWarning, "HttpResponse", "Tried to add header after headers had already been sent."); + return; + } + String header = key + ": " + value + "\r\n"; m_Stream->Write(header.CStr(), header.GetLength()); } diff --git a/lib/remote/httpserverconnection.cpp b/lib/remote/httpserverconnection.cpp index 4348be986..13f321df4 100644 --- a/lib/remote/httpserverconnection.cpp +++ b/lib/remote/httpserverconnection.cpp @@ -157,6 +157,8 @@ void HttpServerConnection::ProcessMessageAsync(HttpRequest& request) try { HttpHandler::ProcessRequest(user, request, response); } catch (const std::exception& ex) { + Log(LogCritical, "HttpServerConnection") + << "Unhandled exception while processing Http request: " << DiagnosticInformation(ex); response.SetStatus(503, "Unhandled exception"); response.AddHeader("Content-Type", "text/plain"); String errorInfo = DiagnosticInformation(ex);