From 3e39ec906e024eb0bc61df0a12e877c6b6755a46 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 19 Jun 2017 13:40:18 +0300 Subject: [PATCH] Change server version from char pointer to char array With this change, it is no longer possible that the server version is deallocated at the very moment it is read. There is still a race, but it's mostly harmless. --- include/maxscale/server.h | 8 +-- server/core/server.cc | 49 +++++++++---------- .../modules/authenticator/MySQLAuth/dbusers.c | 21 ++++---- .../protocol/MySQL/MySQLClient/mysql_client.c | 3 +- 4 files changed, 39 insertions(+), 42 deletions(-) diff --git a/include/maxscale/server.h b/include/maxscale/server.h index c644cb2dc..6c6bc05b4 100644 --- a/include/maxscale/server.h +++ b/include/maxscale/server.h @@ -27,7 +27,9 @@ MXS_BEGIN_DECLS #define MAX_SERVER_ADDRESS_LEN 1024 #define MAX_SERVER_MONUSER_LEN 1024 -#define MAX_SERVER_MONPW_LEN 1024 +#define MAX_SERVER_MONPW_LEN 1024 +#define MAX_SERVER_VERSION_LEN 256 + #define MAX_NUM_SLAVES 128 /**< Maximum number of slaves under a single server*/ /** @@ -88,7 +90,7 @@ typedef struct server SERVER_STATS stats; /**< The server statistics */ struct server *next; /**< Next server */ struct server *nextdb; /**< Next server in list attached to a service */ - char *server_string; /**< Server version string, i.e. MySQL server version */ + char version_string[MAX_SERVER_VERSION_LEN]; /**< Server version string, i.e. MySQL server version */ long node_id; /**< Node id, server_id for M/S or local_index for Galera */ int rlag; /**< Replication Lag for Master / Slave replication */ unsigned long node_ts; /**< Last timestamp set from M/S monitor module */ @@ -308,7 +310,7 @@ extern DCB *server_get_persistent(SERVER *server, const char *user, const char extern void server_update_address(SERVER *server, const char *address); extern void server_update_port(SERVER *server, unsigned short port); extern unsigned int server_map_status(const char *str); -extern bool server_set_version_string(SERVER* server, const char* string); +extern void server_set_version_string(SERVER* server, const char* string); extern void server_set_status(SERVER *server, int bit); extern void server_clear_status(SERVER *server, int bit); diff --git a/server/core/server.cc b/server/core/server.cc index d5be37d62..d64d18b05 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -131,7 +131,6 @@ SERVER* server_alloc(const char *name, const char *address, unsigned short port, server->master_id = -1; server->depth = -1; server->parameters = NULL; - server->server_string = NULL; spinlock_init(&server->lock); server->persistent = persistent; server->persistmax = 0; @@ -187,7 +186,6 @@ server_free(SERVER *tofreeserver) /* Clean up session and free the memory */ MXS_FREE(tofreeserver->protocol); MXS_FREE(tofreeserver->unique_name); - MXS_FREE(tofreeserver->server_string); server_parameter_free(tofreeserver->parameters); if (tofreeserver->persistent) @@ -496,10 +494,7 @@ dprintServer(DCB *dcb, const SERVER *server) MXS_FREE(stat); dcb_printf(dcb, "\tProtocol: %s\n", server->protocol); dcb_printf(dcb, "\tPort: %d\n", server->port); - if (server->server_string) - { - dcb_printf(dcb, "\tServer Version: %s\n", server->server_string); - } + dcb_printf(dcb, "\tServer Version: %s\n", server->version_string); dcb_printf(dcb, "\tNode Id: %ld\n", server->node_id); dcb_printf(dcb, "\tMaster Id: %ld\n", server->master_id); if (server->slaves) @@ -1080,28 +1075,35 @@ server_map_status(const char *str) /** * Set the version string of the server. + * * @param server Server to update - * @param string Version string - * @return True if the assignment of the version string was successful, false if - * memory allocation failed. + * @param version Version string */ -bool server_set_version_string(SERVER* server, const char* string) +void server_set_version_string(SERVER* server, const char* version) { - bool rval = true; - string = MXS_STRDUP(string); + // There is a race here. The string may be accessed, while we are + // updating it. Thus we take some precautions to ensure that the + // string cannot be completely garbled at any point. - if (string) + size_t old_len = strlen(server->version_string); + size_t new_len = strlen(version); + + if (new_len >= MAX_SERVER_VERSION_LEN) { - char* old = server->server_string; - server->server_string = (char*)string; - MXS_FREE(old); - } - else - { - rval = false; + new_len = MAX_SERVER_VERSION_LEN - 1; } - return rval; + if (new_len < old_len) + { + // If the new string is shorter, we start by nulling out the + // excess data. + memset(server->version_string + new_len, 0, old_len - new_len); + } + + strncpy(server->version_string, version, new_len); + // No null-byte needs to be set. The array starts out as all zeros + // and the above memset adds the necessary null, should the new string + // be shorter than the old. } /** @@ -1396,10 +1398,7 @@ static json_t* server_json_attributes(const SERVER* server) json_object_set_new(attr, CN_STATUS, json_string(stat)); MXS_FREE(stat); - if (server->server_string) - { - json_object_set_new(attr, CN_VERSION_STRING, json_string(server->server_string)); - } + json_object_set_new(attr, CN_VERSION_STRING, json_string(server->version_string)); json_object_set_new(attr, "node_id", json_integer(server->node_id)); json_object_set_new(attr, "master_id", json_integer(server->master_id)); diff --git a/server/modules/authenticator/MySQLAuth/dbusers.c b/server/modules/authenticator/MySQLAuth/dbusers.c index 9228edfe7..778fc0324 100644 --- a/server/modules/authenticator/MySQLAuth/dbusers.c +++ b/server/modules/authenticator/MySQLAuth/dbusers.c @@ -522,14 +522,14 @@ static bool check_server_permissions(SERVICE *service, SERVER* server, mysql_get_character_set_info(mysql, &cs_info); server->charset = cs_info.number; - if (server->server_string == NULL) + if (server->version_string[0] == 0) { - const char *server_string = mysql_get_server_info(mysql); - server_set_version_string(server, server_string); + const char *version_string = mysql_get_server_info(mysql); + server_set_version_string(server, version_string); } const char *template = "SELECT user, host, %s, Select_priv FROM mysql.user limit 1"; - const char* query_pw = strstr(server->server_string, "5.7.") ? + const char* query_pw = strstr(server->version_string, "5.7.") ? MYSQL57_PASSWORD : MYSQL_PASSWORD; char query[strlen(template) + strlen(query_pw) + 1]; bool rval = true; @@ -726,18 +726,15 @@ void commit_sqlite_transaction(sqlite3 *handle) } } -int get_users_from_server(MYSQL *con, SERVER_REF *server, SERVICE *service, SERV_LISTENER *listener) +int get_users_from_server(MYSQL *con, SERVER_REF *server_ref, SERVICE *service, SERV_LISTENER *listener) { - if (server->server->server_string == NULL) + if (server_ref->server->version_string[0] == 0) { - const char *server_string = mysql_get_server_info(con); - if (!server_set_version_string(server->server, server_string)) - { - return -1; - } + const char *version_string = mysql_get_server_info(con); + server_set_version_string(server_ref->server, version_string); } - char *query = get_new_users_query(server->server->server_string, service->enable_root); + char *query = get_new_users_query(server_ref->server->version_string, service->enable_root); MYSQL_AUTH *instance = (MYSQL_AUTH*)listener->auth_instance; bool anon_user = false; int users = 0; diff --git a/server/modules/protocol/MySQL/MySQLClient/mysql_client.c b/server/modules/protocol/MySQL/MySQLClient/mysql_client.c index 3421256d4..4082bbf68 100644 --- a/server/modules/protocol/MySQL/MySQLClient/mysql_client.c +++ b/server/modules/protocol/MySQL/MySQLClient/mysql_client.c @@ -224,8 +224,7 @@ int MySQLSendHandshake(DCB* dcb) { mysql_server_language = dcb->service->dbref->server->charset; - if (dcb->service->dbref->server->server_string && - strstr(dcb->service->dbref->server->server_string, "10.2.")) + if (strstr(dcb->service->dbref->server->version_string, "10.2.")) { /** The backend servers support the extended capabilities */ is_maria = true;