From e1f1c8873b6dcfbcf41b01ec71b771c8a3fd0bbd Mon Sep 17 00:00:00 2001 From: Peter Thorson Date: Thu, 11 Dec 2014 09:52:03 -0500 Subject: [PATCH] Major overhaul to handshake internal state machine error checking references #389 #361 This overhaul removes the use of exceptions in handling state errors during handshake processing. All such errors are demoted to rerror/recoverable error and will terminate the connection only rather than the whole application. In addition, there are some cases where we expect to see a state error due to non-deterministic interleaving of timeout handlers and read/write handlers. In these expected cases state errors are ignored entirely. --- websocketpp/connection.hpp | 39 --- websocketpp/impl/connection_impl.hpp | 378 +++++++++++++++------------ 2 files changed, 209 insertions(+), 208 deletions(-) diff --git a/websocketpp/connection.hpp b/websocketpp/connection.hpp index 596c6e0c9..b7780eecb 100644 --- a/websocketpp/connection.hpp +++ b/websocketpp/connection.hpp @@ -1251,45 +1251,6 @@ class connection /// Perform WebSocket handshake validation of m_request using m_processor. /// set m_response and return false on error. bool process_handshake_request(); - - /// Atomically change the internal connection state. - /** - * @param req The required starting state. If the internal state does not - * match req an exception is thrown. - * - * @param dest The state to change to. - * - * @param msg The message to include in the exception thrown - */ - void atomic_state_change(istate_type req, istate_type dest, - std::string msg); - - /// Atomically change the internal and external connection state. - /** - * @param ireq The required starting internal state. If the internal state - * does not match ireq an exception is thrown. - * - * @param idest The internal state to change to. - * - * @param ereq The required starting external state. If the external state - * does not match ereq an exception is thrown. - * - * @param edest The external state to change to. - * - * @param msg The message to include in the exception thrown - */ - void atomic_state_change(istate_type ireq, istate_type idest, - session::state::value ereq, session::state::value edest, - std::string msg); - - /// Atomically read and compared the internal state. - /** - * @param req The state to test against. If the internal state does not - * match req an exception is thrown. - * - * @param msg The message to include in the exception thrown - */ - void atomic_state_check(istate_type req, std::string msg); private: /// Completes m_response, serializes it, and sends it out on the wire. void send_http_response(); diff --git a/websocketpp/impl/connection_impl.hpp b/websocketpp/impl/connection_impl.hpp index 54991f326..505501653 100644 --- a/websocketpp/impl/connection_impl.hpp +++ b/websocketpp/impl/connection_impl.hpp @@ -105,10 +105,12 @@ lib::error_code connection::send(typename config::message_type::ptr msg) if (m_alog.static_test(log::alevel::devel)) { m_alog.write(log::alevel::devel,"connection send"); } - // TODO: - if (m_state != session::state::open) { - return error::make_error_code(error::invalid_state); + { + scoped_lock_type lock(m_connection_state_lock); + if (m_state != session::state::open) { + return error::make_error_code(error::invalid_state); + } } message_ptr outgoing_msg; @@ -154,9 +156,15 @@ void connection::ping(std::string const& payload, lib::error_code& ec) { m_alog.write(log::alevel::devel,"connection ping"); } - if (m_state != session::state::open) { - ec = error::make_error_code(error::invalid_state); - return; + { + scoped_lock_type lock(m_connection_state_lock); + if (m_state != session::state::open) { + std::stringstream ss; + ss << "connection::ping called from invalid state " << m_state; + m_alog.write(log::alevel::devel,ss.str()); + ec = error::make_error_code(error::invalid_state); + return; + } } message_ptr msg = m_msg_manager->get_message(); @@ -245,9 +253,15 @@ void connection::pong(std::string const& payload, lib::error_code& ec) { m_alog.write(log::alevel::devel,"connection pong"); } - if (m_state != session::state::open) { - ec = error::make_error_code(error::invalid_state); - return; + { + scoped_lock_type lock(m_connection_state_lock); + if (m_state != session::state::open) { + std::stringstream ss; + ss << "connection::pong called from invalid state " << m_state; + m_alog.write(log::alevel::devel,ss.str()); + ec = error::make_error_code(error::invalid_state); + return; + } } message_ptr msg = m_msg_manager->get_message(); @@ -293,15 +307,17 @@ void connection::close(close::status::value const code, m_alog.write(log::alevel::devel,"connection close"); } + // Truncate reason to maximum size allowable in a close frame. + std::string tr(reason,0,std::min(reason.size(), + frame::limits::close_reason_size)); + + scoped_lock_type lock(m_connection_state_lock); + if (m_state != session::state::open) { ec = error::make_error_code(error::invalid_state); return; } - // Truncate reason to maximum size allowable in a close frame. - std::string tr(reason,0,std::min(reason.size(), - frame::limits::close_reason_size)); - ec = this->send_close_frame(code,tr,false,close::status::terminal(code)); } @@ -526,21 +542,16 @@ connection::get_response_header(std::string const & key) const { template void connection::set_status(http::status_code::value code) { - //scoped_lock_type lock(m_connection_state_lock); - if (m_internal_state != istate::PROCESS_HTTP_REQUEST) { throw exception("Call to set_status from invalid state", error::make_error_code(error::invalid_state)); } - m_response.set_status(code); } template void connection::set_status(http::status_code::value code, std::string const & msg) { - //scoped_lock_type lock(m_connection_state_lock); - if (m_internal_state != istate::PROCESS_HTTP_REQUEST) { throw exception("Call to set_status from invalid state", error::make_error_code(error::invalid_state)); @@ -550,8 +561,6 @@ void connection::set_status(http::status_code::value code, } template void connection::set_body(std::string const & value) { - //scoped_lock_type lock(m_connection_state_lock); - if (m_internal_state != istate::PROCESS_HTTP_REQUEST) { throw exception("Call to set_status from invalid state", error::make_error_code(error::invalid_state)); @@ -564,8 +573,6 @@ template void connection::append_header(std::string const & key, std::string const & val) { - //scoped_lock_type lock(m_connection_state_lock); - if (m_is_server) { if (m_internal_state == istate::PROCESS_HTTP_REQUEST) { // we are setting response headers for an incoming server connection @@ -588,8 +595,6 @@ template void connection::replace_header(std::string const & key, std::string const & val) { - // scoped_lock_type lock(m_connection_state_lock); - if (m_is_server) { if (m_internal_state == istate::PROCESS_HTTP_REQUEST) { // we are setting response headers for an incoming server connection @@ -611,8 +616,6 @@ void connection::replace_header(std::string const & key, template void connection::remove_header(std::string const & key) { - //scoped_lock_type lock(m_connection_state_lock); - if (m_is_server) { if (m_internal_state == istate::PROCESS_HTTP_REQUEST) { // we are setting response headers for an incoming server connection @@ -643,11 +646,13 @@ template void connection::start() { m_alog.write(log::alevel::devel,"connection start"); - this->atomic_state_change( - istate::USER_INIT, - istate::TRANSPORT_INIT, - "Start must be called from user init state" - ); + if (m_internal_state != istate::USER_INIT) { + m_alog.write(log::alevel::devel,"Start called in invalid state"); + this->terminate(error::make_error_code(error::invalid_state)); + return; + } + + m_internal_state = istate::TRANSPORT_INIT; // Depending on how the transport implements init this function may return // immediately and call handle_transport_init later or call @@ -665,39 +670,31 @@ template void connection::handle_transport_init(lib::error_code const & ec) { m_alog.write(log::alevel::devel,"connection handle_transport_init"); - { - scoped_lock_type lock(m_connection_state_lock); - - if (m_internal_state != istate::TRANSPORT_INIT) { - throw exception("handle_transport_init must be called from transport init state", - error::make_error_code(error::invalid_state)); - } + lib::error_code ecm = ec; - if (!ec) { - // unless there was a transport error, advance internal state. - if (m_is_server) { - m_internal_state = istate::READ_HTTP_REQUEST; - } else { - m_internal_state = istate::WRITE_HTTP_REQUEST; - } - } + if (m_internal_state != istate::TRANSPORT_INIT) { + m_alog.write(log::alevel::devel, + "handle_transport_init must be called from transport init state"); + ecm = error::make_error_code(error::invalid_state); } - if (ec) { + if (ecm) { std::stringstream s; - s << "handle_transport_init received error: "<< ec.message(); - m_elog.write(log::elevel::fatal,s.str()); + s << "handle_transport_init received error: "<< ecm.message(); + m_elog.write(log::elevel::rerror,s.str()); - this->terminate(ec); + this->terminate(ecm); return; } // At this point the transport is ready to read and write bytes. if (m_is_server) { + m_internal_state = istate::READ_HTTP_REQUEST; this->read_handshake(1); } else { // We are a client. Set the processor to the version specified in the // config file and send a handshake request. + m_internal_state = istate::WRITE_HTTP_REQUEST; m_processor = get_processor(config::client_version); this->send_http_request(); } @@ -739,24 +736,37 @@ void connection::handle_read_handshake(lib::error_code const & ec, { m_alog.write(log::alevel::devel,"connection handle_read_handshake"); - this->atomic_state_check( - istate::READ_HTTP_REQUEST, - "handle_read_handshake must be called from READ_HTTP_REQUEST state" - ); + lib::error_code ecm = ec; - if (ec) { - if (ec == transport::error::eof) { - // we expect to get eof if the connection is closed already - if (m_state == session::state::closed) { - m_alog.write(log::alevel::devel,"got eof from closed con"); - return; - } + if (!ecm) { + scoped_lock_type lock(m_connection_state_lock); + + if (m_state == session::state::connecting) { + if (m_internal_state != istate::READ_HTTP_REQUEST) { + ecm = error::make_error_code(error::invalid_state); + } + } else if (m_state == session::state::closed) { + // The connection was canceled while the response was being sent, + // usually by the handshake timer. This is basically expected + // (though hopefully rare) and there is nothing we can do so ignore. + m_alog.write(log::alevel::devel, + "handle_send_http_response invoked after connection was closed"); + return; + } else { + ecm = error::make_error_code(error::invalid_state); } + } - std::stringstream s; - s << "error in handle_read_handshake: "<< ec.message(); - m_elog.write(log::elevel::fatal,s.str()); - this->terminate(ec); + if (ecm) { + if (ecm == transport::error::eof && m_state == session::state::closed) { + // we expect to get eof if the connection is closed already + m_alog.write(log::alevel::devel, + "got (expected) eof/state error from closed con"); + return; + } + + log_err(log::elevel::rerror,"handle_read_handshake",ecm); + this->terminate(ecm); return; } @@ -831,12 +841,9 @@ void connection::handle_read_handshake(lib::error_code const & ec, std::copy(m_buf+bytes_processed,m_buf+bytes_transferred,m_buf); m_buf_cursor = bytes_transferred-bytes_processed; - this->atomic_state_change( - istate::READ_HTTP_REQUEST, - istate::PROCESS_HTTP_REQUEST, - "send_http_response must be called from READ_HTTP_REQUEST state" - ); + m_internal_state = istate::PROCESS_HTTP_REQUEST; + // We have the complete request. Process it. this->process_handshake_request(); this->send_http_response(); @@ -863,11 +870,15 @@ void connection::handle_read_handshake(lib::error_code const & ec, // state and calls send_http_response template void connection::send_http_response_error() { - this->atomic_state_change( - istate::READ_HTTP_REQUEST, - istate::PROCESS_HTTP_REQUEST, - "send_http_response must be called from READ_HTTP_REQUEST state" - ); + if (m_internal_state != istate::READ_HTTP_REQUEST) { + m_alog.write(log::alevel::devel, + "send_http_response_error called in invalid state"); + this->terminate(error::make_error_code(error::invalid_state)); + return; + } + + m_internal_state = istate::PROCESS_HTTP_REQUEST; + this->send_http_response(); } @@ -879,15 +890,16 @@ void connection::handle_read_frame(lib::error_code const & ec, { //m_alog.write(log::alevel::devel,"connection handle_read_frame"); - this->atomic_state_check( - istate::PROCESS_CONNECTION, - "handle_read_frame must be called from PROCESS_CONNECTION state" - ); + lib::error_code ecm = ec; - if (ec) { - log::level echannel = log::elevel::fatal; + if (!ecm && m_internal_state != istate::PROCESS_CONNECTION) { + ecm = error::make_error_code(error::invalid_state); + } + + if (ecm) { + log::level echannel = log::elevel::rerror; - if (ec == transport::error::eof) { + if (ecm == transport::error::eof) { if (m_state == session::state::closed) { // we expect to get eof if the connection is closed already // just ignore it @@ -900,8 +912,17 @@ void connection::handle_read_frame(lib::error_code const & ec, terminate(lib::error_code()); return; } - } - if (ec == transport::error::tls_short_read) { + } else if (ecm == error::invalid_state) { + // In general, invalid state errors in the closed state are the + // result of handlers that were in the system already when the state + // changed and should be ignored as they pose no problems and there + // is nothing useful that we can do about them. + if (m_state == session::state::closed) { + m_alog.write(log::alevel::devel, + "handle_read_frame: got invalid istate in closed state"); + return; + } + } else if (ecm == transport::error::tls_short_read) { if (m_state == session::state::closed) { // We expect to get a TLS short read if we try to read after the // connection is closed. If this happens ignore and exit the @@ -910,12 +931,12 @@ void connection::handle_read_frame(lib::error_code const & ec, return; } echannel = log::elevel::rerror; - } else if (ec == transport::error::action_after_shutdown) { + } else if (ecm == transport::error::action_after_shutdown) { echannel = log::elevel::info; } - log_err(echannel, "handle_read_frame", ec); - this->terminate(ec); + log_err(echannel, "handle_read_frame", ecm); + this->terminate(ecm); return; } @@ -1230,24 +1251,47 @@ template void connection::handle_send_http_response(lib::error_code const & ec) { m_alog.write(log::alevel::devel,"handle_send_http_response"); - this->atomic_state_check( - istate::PROCESS_HTTP_REQUEST, - "handle_send_http_response must be called from PROCESS_HTTP_REQUEST state" - ); + lib::error_code ecm = ec; - if (ec) { - log_err(log::elevel::rerror,"handle_send_http_response",ec); - this->terminate(ec); - return; + if (!ecm) { + scoped_lock_type lock(m_connection_state_lock); + + if (m_state == session::state::connecting) { + if (m_internal_state != istate::PROCESS_HTTP_REQUEST) { + ecm = error::make_error_code(error::invalid_state); + } + } else if (m_state == session::state::closed) { + // The connection was canceled while the response was being sent, + // usually by the handshake timer. This is basically expected + // (though hopefully rare) and there is nothing we can do so ignore. + m_alog.write(log::alevel::devel, + "handle_send_http_response invoked after connection was closed"); + return; + } else { + ecm = error::make_error_code(error::invalid_state); + } } - this->log_open_result(); + if (ecm) { + if (ecm == transport::error::eof && m_state == session::state::closed) { + // we expect to get eof if the connection is closed already + m_alog.write(log::alevel::devel, + "got (expected) eof/state error from closed con"); + return; + } + + log_err(log::elevel::rerror,"handle_send_http_response",ecm); + this->terminate(ecm); + return; + } if (m_handshake_timer) { m_handshake_timer->cancel(); m_handshake_timer.reset(); } + this->log_open_result(); + if (m_response.get_status_code() != http::status_code::switching_protocols) { if (m_processor) { @@ -1264,13 +1308,8 @@ void connection::handle_send_http_response(lib::error_code const & ec) { return; } - this->atomic_state_change( - istate::PROCESS_HTTP_REQUEST, - istate::PROCESS_CONNECTION, - session::state::connecting, - session::state::open, - "handle_send_http_response must be called from PROCESS_HTTP_REQUEST state" - ); + m_internal_state = istate::PROCESS_CONNECTION; + m_state = session::state::open; if (m_open_handler) { m_open_handler(m_connection_hdl); @@ -1342,22 +1381,41 @@ template void connection::handle_send_http_request(lib::error_code const & ec) { m_alog.write(log::alevel::devel,"handle_send_http_request"); - this->atomic_state_check( - istate::WRITE_HTTP_REQUEST, - "handle_send_http_request must be called from WRITE_HTTP_REQUEST state" - ); + lib::error_code ecm = ec; - if (ec) { - log_err(log::elevel::rerror,"handle_send_http_request",ec); - this->terminate(ec); - return; + if (!ecm) { + scoped_lock_type lock(m_connection_state_lock); + + if (m_state == session::state::connecting) { + if (m_internal_state != istate::WRITE_HTTP_REQUEST) { + ecm = error::make_error_code(error::invalid_state); + } else { + m_internal_state = istate::READ_HTTP_RESPONSE; + } + } else if (m_state == session::state::closed) { + // The connection was canceled while the response was being sent, + // usually by the handshake timer. This is basically expected + // (though hopefully rare) and there is nothing we can do so ignore. + m_alog.write(log::alevel::devel, + "handle_send_http_request invoked after connection was closed"); + return; + } else { + ecm = error::make_error_code(error::invalid_state); + } } - this->atomic_state_change( - istate::WRITE_HTTP_REQUEST, - istate::READ_HTTP_RESPONSE, - "handle_send_http_request must be called from WRITE_HTTP_REQUEST state" - ); + if (ecm) { + if (ecm == transport::error::eof && m_state == session::state::closed) { + // we expect to get eof if the connection is closed already + m_alog.write(log::alevel::devel, + "got (expected) eof/state error from closed con"); + return; + } + + log_err(log::elevel::rerror,"handle_send_http_request",ecm); + this->terminate(ecm); + return; + } transport_con_type::async_read_at_least( 1, @@ -1378,16 +1436,40 @@ void connection::handle_read_http_response(lib::error_code const & ec, { m_alog.write(log::alevel::devel,"handle_read_http_response"); - this->atomic_state_check( - istate::READ_HTTP_RESPONSE, - "handle_read_http_response must be called from READ_HTTP_RESPONSE state" - ); + lib::error_code ecm = ec; - if (ec) { - log_err(log::elevel::rerror,"handle_send_http_response",ec); - this->terminate(ec); + if (!ecm) { + scoped_lock_type lock(m_connection_state_lock); + + if (m_state == session::state::connecting) { + if (m_internal_state != istate::READ_HTTP_RESPONSE) { + ecm = error::make_error_code(error::invalid_state); + } + } else if (m_state == session::state::closed) { + // The connection was canceled while the response was being sent, + // usually by the handshake timer. This is basically expected + // (though hopefully rare) and there is nothing we can do so ignore. + m_alog.write(log::alevel::devel, + "handle_read_http_response invoked after connection was closed"); + return; + } else { + ecm = error::make_error_code(error::invalid_state); + } + } + + if (ecm) { + if (ecm == transport::error::eof && m_state == session::state::closed) { + // we expect to get eof if the connection is closed already + m_alog.write(log::alevel::devel, + "got (expected) eof/state error from closed con"); + return; + } + + log_err(log::elevel::rerror,"handle_read_http_response",ecm); + this->terminate(ecm); return; } + size_t bytes_processed = 0; // TODO: refactor this to use error codes rather than exceptions try { @@ -1417,14 +1499,9 @@ void connection::handle_read_http_response(lib::error_code const & ec, return; } - // response is valid, connection can now be assumed to be open - this->atomic_state_change( - istate::READ_HTTP_RESPONSE, - istate::PROCESS_CONNECTION, - session::state::connecting, - session::state::open, - "handle_read_http_response must be called from READ_HTTP_RESPONSE state" - ); + // response is valid, connection can now be assumed to be open + m_internal_state = istate::PROCESS_CONNECTION; + m_state = session::state::open; this->log_open_result(); @@ -1505,6 +1582,7 @@ void connection::terminate(lib::error_code const & ec) { m_local_close_reason = ec.message(); } + // TODO: does this need a mutex? if (m_state == session::state::connecting) { m_state = session::state::closed; tstat = failed; @@ -1708,44 +1786,6 @@ void connection::handle_write_frame(lib::error_code const & ec) } } -template -void connection::atomic_state_change(istate_type req, istate_type dest, - std::string msg) -{ - scoped_lock_type lock(m_connection_state_lock); - - if (m_internal_state != req) { - throw exception(msg,error::make_error_code(error::invalid_state)); - } - - m_internal_state = dest; -} - -template -void connection::atomic_state_change(istate_type internal_req, - istate_type internal_dest, session::state::value external_req, - session::state::value external_dest, std::string msg) -{ - scoped_lock_type lock(m_connection_state_lock); - - if (m_internal_state != internal_req || m_state != external_req) { - throw exception(msg,error::make_error_code(error::invalid_state)); - } - - m_internal_state = internal_dest; - m_state = external_dest; -} - -template -void connection::atomic_state_check(istate_type req, std::string msg) -{ - scoped_lock_type lock(m_connection_state_lock); - - if (m_internal_state != req) { - throw exception(msg,error::make_error_code(error::invalid_state)); - } -} - template std::vector const & connection::get_supported_versions() const {