From 782b8db2aadc98e12af64ad359b01c4ff453fe73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 31 Jul 2017 23:29:48 +0300 Subject: [PATCH] Fix readwritesplit handling of unexpected responses The backend server can send a response even if the client hasn't sent a request. One case where this occurs is when the server is shutting down. The internal logic of readwritesplit can't handle unexpected states gracefully so the safest thing to do is to just ignore them and send the responses to the client. --- include/maxscale/protocol/mysql.h | 15 ++++++- server/modules/protocol/MySQL/mysql_common.c | 23 ++++------ .../routing/readwritesplit/readwritesplit.cc | 45 ++++++++++++++++++- 3 files changed, 67 insertions(+), 16 deletions(-) diff --git a/include/maxscale/protocol/mysql.h b/include/maxscale/protocol/mysql.h index 90795e36b..33386ec42 100644 --- a/include/maxscale/protocol/mysql.h +++ b/include/maxscale/protocol/mysql.h @@ -454,9 +454,22 @@ mxs_auth_state_t gw_send_backend_auth(DCB *dcb); /** Write an OK packet to a DCB */ int mxs_mysql_send_ok(DCB *dcb, int sequence, uint8_t affected_rows, const char* message); -/** Check for OK packet */ +/** + * @brief Check if the buffer contains an OK packet + * + * @param buffer Buffer containing a complete MySQL packet + * @return True if the buffer contains an OK packet + */ bool mxs_mysql_is_ok_packet(GWBUF *buffer); +/** + * @brief Check if the buffer contains an ERR packet + * + * @param buffer Buffer containing a complete MySQL packet + * @return True if the buffer contains an ERR packet + */ +bool mxs_mysql_is_err_packet(GWBUF *buffer); + /** * @brief Check if a buffer contains a result set * diff --git a/server/modules/protocol/MySQL/mysql_common.c b/server/modules/protocol/MySQL/mysql_common.c index ecf7fb684..e7803e9c9 100644 --- a/server/modules/protocol/MySQL/mysql_common.c +++ b/server/modules/protocol/MySQL/mysql_common.c @@ -1498,23 +1498,18 @@ bool gw_read_backend_handshake(DCB *dcb, GWBUF *buffer) return rval; } -/** - * @brief Check if the buffer contains an OK packet - * - * @param buffer Buffer containing a complete MySQL packet - * @return True if the buffer contains an OK packet - */ bool mxs_mysql_is_ok_packet(GWBUF *buffer) { - bool rval = false; - uint8_t cmd; + uint8_t cmd = 0xff; // Default should differ from the OK packet + gwbuf_copy_data(buffer, MYSQL_HEADER_LEN, 1, &cmd); + return cmd == MYSQL_REPLY_OK; +} - if (gwbuf_copy_data(buffer, MYSQL_HEADER_LEN, 1, &cmd) && cmd == MYSQL_REPLY_OK) - { - rval = true; - } - - return rval; +bool mxs_mysql_is_err_packet(GWBUF *buffer) +{ + uint8_t cmd = 0x00; // Default should differ from the ERR packet + gwbuf_copy_data(buffer, MYSQL_HEADER_LEN, 1, &cmd); + return cmd == MYSQL_REPLY_ERR; } bool mxs_mysql_is_result_set(GWBUF *buffer) diff --git a/server/modules/routing/readwritesplit/readwritesplit.cc b/server/modules/routing/readwritesplit/readwritesplit.cc index a1390a8fa..d6fad38dc 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.cc +++ b/server/modules/routing/readwritesplit/readwritesplit.cc @@ -1076,6 +1076,40 @@ static json_t* diagnostics_json(const MXS_ROUTER *instance) return rval; } +static void log_unexpected_response(DCB* dcb, GWBUF* buffer) +{ + if (mxs_mysql_is_err_packet(buffer)) + { + /** This should be the only valid case where the server sends a response + * without the client sending one first. MaxScale does not yet advertise + * the progress reporting flag so we don't need to handle it. */ + uint8_t* data = GWBUF_DATA(buffer); + size_t len = MYSQL_GET_PAYLOAD_LEN(data); + uint16_t errcode = MYSQL_GET_ERRCODE(data); + std::string errstr((char*)data + 7, (char*)data + 7 + len - 3); + + if (errcode == ER_CONNECTION_KILLED) + { + MXS_INFO("Connection from '%s'@'%s' to '%s' was killed", + dcb->session->client_dcb->user, + dcb->session->client_dcb->remote, + dcb->server->unique_name); + } + else + { + MXS_WARNING("Server '%s' sent an unexpected error: %hu, %s", + dcb->server->unique_name, errcode, errstr.c_str()); + } + } + else + { + MXS_ERROR("Unexpected internal state: received response 0x%02hhx from " + "server '%s' when no response was expected", + mxs_mysql_get_command(buffer), dcb->server->unique_name); + ss_dassert(false); + } +} + /** * @brief Client Reply routine * @@ -1102,9 +1136,18 @@ static void clientReply(MXS_ROUTER *instance, SRWBackend backend = get_backend_from_dcb(rses, backend_dcb); + if (backend->get_reply_state() == REPLY_STATE_DONE) + { + /** If we receive an unexpected response from the server, the internal + * logic cannot handle this situation. Routing the reply straight to + * the client should be the safest thing to do at this point. */ + log_unexpected_response(backend_dcb, writebuf); + MXS_SESSION_ROUTE_REPLY(backend_dcb->session, writebuf); + return; + } + /** Statement was successfully executed, free the stored statement */ session_clear_stmt(backend_dcb->session); - ss_dassert(backend->get_reply_state() != REPLY_STATE_DONE); if (reply_is_complete(backend, writebuf)) {