From 1c733bf4501d897206e5e5786be3c8b6091f536e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Aug 2018 12:44:25 +0300 Subject: [PATCH 01/17] MXS-2015: Always check for error responses As a JSON object can never start with the ERR prefix, it is safe to check it for all requests. Also fixed the missing newline in the avrorouter error message. --- connectors/cdc-connector/cdc_connector.cpp | 2 +- server/modules/routing/avrorouter/avro_client.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index be751488a..9286aeff4 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -584,7 +584,7 @@ bool Connection::read_row(std::string& dest) m_buf_ptr = m_buffer.begin(); } - if (!m_connected && is_error(dest.c_str())) + if (is_error(dest.c_str())) { rval = false; } diff --git a/server/modules/routing/avrorouter/avro_client.c b/server/modules/routing/avrorouter/avro_client.c index bc454477d..13eb037ba 100644 --- a/server/modules/routing/avrorouter/avro_client.c +++ b/server/modules/routing/avrorouter/avro_client.c @@ -484,7 +484,7 @@ avro_client_process_command(AVRO_INSTANCE *router, AVRO_CLIENT *client, GWBUF *q } else { - dcb_printf(client->dcb, "ERR REQUEST-DATA with no data"); + dcb_printf(client->dcb, "ERR REQUEST-DATA with no data\n"); } } /* Return last GTID info */ From d6ed10975e710a092bd96b676722da6a375f415e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Aug 2018 16:53:58 +0300 Subject: [PATCH 02/17] MXS-2015: Add missing newlines The newlines were missing from a few of the avrorouter responses. --- server/modules/routing/avrorouter/avro_client.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_client.c b/server/modules/routing/avrorouter/avro_client.c index 13eb037ba..05b98f747 100644 --- a/server/modules/routing/avrorouter/avro_client.c +++ b/server/modules/routing/avrorouter/avro_client.c @@ -479,7 +479,7 @@ avro_client_process_command(AVRO_INSTANCE *router, AVRO_CLIENT *client, GWBUF *q } else { - dcb_printf(client->dcb, "ERR NO-FILE File '%s' not found.", client->avro_binfile); + dcb_printf(client->dcb, "ERR NO-FILE File '%s' not found.\n", client->avro_binfile); } } else @@ -825,8 +825,7 @@ static bool avro_client_stream_data(AVRO_CLIENT *client) } else { - fprintf(stderr, "No file specified\n"); - dcb_printf(client->dcb, "ERR avro file not specified"); + dcb_printf(client->dcb, "ERR avro file not specified\n"); } return read_more; From fb9ffdc753d0c6ce5e1c38c7b62799a221cad717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Aug 2018 17:26:15 +0300 Subject: [PATCH 03/17] MXS-2015: Improve CDC connector error messages The error messages now display the data received so far. This will help resolve at which point the streaming process was when an error occurs. --- connectors/cdc-connector/cdc_connector.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index 9286aeff4..aa014b56e 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -280,6 +281,11 @@ bool Connection::connect(const std::string& table, const std::string& gtid) { rval = true; } + else + { + m_error += ". Data received so far: "; + std::copy(m_buffer.begin(), m_buffer.end(), std::back_inserter(m_error)); + } } } } From 57334153fe1287a96fb2a19c73ac15c82d5950f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 15 Aug 2018 20:23:46 +0300 Subject: [PATCH 04/17] MXS-1880: Fix crash after restart The offsets in the events were wrong if MaxScale was restarted in the middle of a binlog. This was caused by the fact that the FDE event contains information on binlog checksums which is required for a correct interpretation of the binlog. The FDE event was only read when the binlog was first opened when it needs to be read every time the binlog is opened. --- server/modules/routing/avrorouter/avro_file.c | 64 ++++++++++++++----- 1 file changed, 47 insertions(+), 17 deletions(-) diff --git a/server/modules/routing/avrorouter/avro_file.c b/server/modules/routing/avrorouter/avro_file.c index 404737d72..d0772aeb9 100644 --- a/server/modules/routing/avrorouter/avro_file.c +++ b/server/modules/routing/avrorouter/avro_file.c @@ -481,6 +481,45 @@ void do_checkpoint(AVRO_INSTANCE *router, uint64_t *total_rows, uint64_t *total_ router->row_count = router->trx_count = 0; } +void process_fde(AVRO_INSTANCE* router, uint8_t* ptr, uint32_t event_size) +{ + const int BLRM_FDE_EVENT_TYPES_OFFSET = 2 + 50 + 4 + 1; + const int FDE_EXTRA_BYTES = 5; + int event_header_length = ptr[BLRM_FDE_EVENT_TYPES_OFFSET - 1]; + int n_events = event_size - event_header_length - BLRM_FDE_EVENT_TYPES_OFFSET - FDE_EXTRA_BYTES; + uint8_t* checksum = ptr + event_size - event_header_length - FDE_EXTRA_BYTES; + + // Precaution to prevent writing too much in case new events are added + int real_len = MXS_MIN(n_events, sizeof(router->event_type_hdr_lens)); + memcpy(router->event_type_hdr_lens, ptr + BLRM_FDE_EVENT_TYPES_OFFSET, real_len); + + router->event_types = n_events; + router->binlog_checksum = checksum[0]; +} + +bool read_fde(AVRO_INSTANCE* router) +{ + bool rval = false; + uint8_t hdbuf[BINLOG_EVENT_HDR_LEN]; + errno = 0; // Helps prevent false positives + + if (pread(router->binlog_fd, hdbuf, BINLOG_EVENT_HDR_LEN, 4) == BINLOG_EVENT_HDR_LEN) + { + uint32_t event_size = extract_field(&hdbuf[9], 32); + uint8_t evbuf[event_size]; + uint8_t event_type = hdbuf[4]; + + if (event_type == FORMAT_DESCRIPTION_EVENT && + pread(router->binlog_fd, evbuf, event_size, 4 + BINLOG_EVENT_HDR_LEN) == event_size) + { + process_fde(router, evbuf, event_size); + rval = true; + } + } + + return rval; +} + /** * @brief Read all replication events from a binlog file. * @@ -512,6 +551,13 @@ avro_binlog_end_t avro_read_all_events(AVRO_INSTANCE *router) return AVRO_BINLOG_ERROR; } + if (!read_fde(router)) + { + MXS_ERROR("Failed to read the FDE event from the binary log: %d, %s", + errno, mxs_strerror(errno)); + return AVRO_BINLOG_ERROR; + } + while (!router->service->svc_do_shutdown) { int n; @@ -638,24 +684,8 @@ avro_binlog_end_t avro_read_all_events(AVRO_INSTANCE *router) hdr.event_size -= 4; } - /* check for FORMAT DESCRIPTION EVENT */ - if (hdr.event_type == FORMAT_DESCRIPTION_EVENT) - { - const int BLRM_FDE_EVENT_TYPES_OFFSET = 2 + 50 + 4 + 1; - const int FDE_EXTRA_BYTES = 5; - int event_header_length = ptr[BLRM_FDE_EVENT_TYPES_OFFSET - 1]; - int n_events = hdr.event_size - event_header_length - BLRM_FDE_EVENT_TYPES_OFFSET - FDE_EXTRA_BYTES; - uint8_t* checksum = ptr + hdr.event_size - event_header_length - FDE_EXTRA_BYTES; - - // Precaution to prevent writing too much in case new events are added - int real_len = MXS_MIN(n_events, sizeof(router->event_type_hdr_lens)); - memcpy(router->event_type_hdr_lens, ptr + BLRM_FDE_EVENT_TYPES_OFFSET, real_len); - - router->event_types = n_events; - router->binlog_checksum = checksum[0]; - } /* Decode CLOSE/STOP Event */ - else if (hdr.event_type == STOP_EVENT) + if (hdr.event_type == STOP_EVENT) { char next_file[BLRM_BINLOG_NAME_STR_LEN + 1]; stop_seen = true; From c38dcff53c605caea55eead5a5d0e72f2c399665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sun, 19 Aug 2018 21:56:53 +0300 Subject: [PATCH 05/17] MXS-2019: Remove header and footer output Now that the exit handlers work correctly, the output of `maxscale --version` no longer consists of a single line but multiple lines in both stdout and stderr. Removing the printing to stderr guarantees that the correct output is produced. --- server/core/gateway.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/core/gateway.cc b/server/core/gateway.cc index e229f9612..1d12f879f 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -1436,7 +1436,7 @@ int main(int argc, char **argv) sigset_t sigpipe_mask; sigset_t saved_mask; bool to_stdout = false; - void (*exitfunp[4])(void) = { mxs_log_finish, cleanup_process_datadir, write_footer, NULL }; + void (*exitfunp[4])(void) = { mxs_log_finish, cleanup_process_datadir, NULL }; int numlocks = 0; bool pid_file_created = false; Worker* worker; @@ -1454,7 +1454,6 @@ int main(int argc, char **argv) progname = *argv; snprintf(datadir, PATH_MAX, "%s", default_datadir); datadir[PATH_MAX] = '\0'; - file_write_header(stderr); // Option string for getopt const char accepted_opts[] = "dnce:f:g:l:vVs:S:?L:D:C:B:U:A:P:G:N:E:F:M:H:p"; From 9663c52f50dcf064ef790e5b5de54619ad7ce394 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 20 Aug 2018 13:04:49 +0300 Subject: [PATCH 06/17] MXS-2015: Remove buffer iterator The iterator to the buffer isn't really needed as the beginning of the buffer can be used instead. This should make the code more robust. Changed the internal buffer from a vector to a deque as the latter is more appropriate for insertion on one end and consumption on the other. --- connectors/cdc-connector/cdc_connector.cpp | 34 +++++++++------------- connectors/cdc-connector/cdc_connector.h | 4 +-- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index aa014b56e..00eb808dc 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -186,7 +186,6 @@ Connection::Connection(const std::string& address, m_timeout(timeout), m_connected(false) { - m_buf_ptr = m_buffer.begin(); } Connection::~Connection() @@ -283,8 +282,9 @@ bool Connection::connect(const std::string& table, const std::string& gtid) } else { - m_error += ". Data received so far: "; + m_error += ". Data received so far: '"; std::copy(m_buffer.begin(), m_buffer.end(), std::back_inserter(m_error)); + m_error += "'"; } } } @@ -545,11 +545,13 @@ bool Connection::read_row(std::string& dest) { if (!m_buffer.empty()) { - std::vector::iterator it = std::find(m_buf_ptr, m_buffer.end(), '\n'); + std::deque::iterator it = std::find(m_buffer.begin(), m_buffer.end(), '\n'); + if (it != m_buffer.end()) { - dest.assign(m_buf_ptr, it); - m_buf_ptr = it + 1; + dest.assign(m_buffer.begin(), it); + m_buffer.erase(m_buffer.begin(), std::next(it)); + assert(m_buffer.empty() || m_buffer[0] != '\n'); break; } } @@ -572,22 +574,14 @@ bool Connection::read_row(std::string& dest) break; } - if (!m_connected) - { - // This is here to work around a missing newline in MaxScale error messages - buf[rc] = '\0'; - - if (is_error(buf)) - { - rval = false; - break; - } - } - - m_buffer.erase(m_buffer.begin(), m_buf_ptr); assert(std::find(m_buffer.begin(), m_buffer.end(), '\n') == m_buffer.end()); - m_buffer.insert(m_buffer.end(), buf, buf + rc); - m_buf_ptr = m_buffer.begin(); + std::copy(buf, buf + rc, std::back_inserter(m_buffer)); + + if (is_error(&m_buffer[0])) + { + rval = false; + break; + } } if (is_error(dest.c_str())) diff --git a/connectors/cdc-connector/cdc_connector.h b/connectors/cdc-connector/cdc_connector.h index fa01ac28b..e91c371f1 100644 --- a/connectors/cdc-connector/cdc_connector.h +++ b/connectors/cdc-connector/cdc_connector.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -137,8 +138,7 @@ private: SValueVector m_keys; SValueVector m_types; int m_timeout; - std::vector m_buffer; - std::vector::iterator m_buf_ptr; + std::deque m_buffer; SRow m_first_row; bool m_connected; From 448099508984038bac1493097f5b1daf331e0baf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 21 Aug 2018 08:37:53 +0300 Subject: [PATCH 07/17] MXS-1996: Remove misleading errors The errors are expected if the file is empty. --- avro/maxavro_file.c | 1 - server/modules/routing/avrorouter/avro_index.c | 4 ---- 2 files changed, 5 deletions(-) diff --git a/avro/maxavro_file.c b/avro/maxavro_file.c index df5292099..48e974498 100644 --- a/avro/maxavro_file.c +++ b/avro/maxavro_file.c @@ -317,7 +317,6 @@ MAXAVRO_FILE* maxavro_file_open(const char* filename) } else { - MXS_ERROR("Failed to initialize avrofile."); maxavro_schema_free(avrofile->schema); error = true; } diff --git a/server/modules/routing/avrorouter/avro_index.c b/server/modules/routing/avrorouter/avro_index.c index 4de727e3d..bfe884582 100644 --- a/server/modules/routing/avrorouter/avro_index.c +++ b/server/modules/routing/avrorouter/avro_index.c @@ -172,10 +172,6 @@ void avro_index_file(AVRO_INSTANCE *router, const char* filename) maxavro_file_close(file); } - else - { - MXS_ERROR("Failed to open file '%s' when generating file index.", filename); - } } /** From 69722a32cafb61910d2d1ee6bdb449dbc49738b1 Mon Sep 17 00:00:00 2001 From: Marko Date: Wed, 22 Aug 2018 22:46:46 +0300 Subject: [PATCH 08/17] Fix crash caused by wildcards in NamedServeFilter source parameter Use the formated IP address instead of the one with wildcard symbols. --- server/modules/filter/namedserverfilter/namedserverfilter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/filter/namedserverfilter/namedserverfilter.cc b/server/modules/filter/namedserverfilter/namedserverfilter.cc index 58ad32731..9f2b94e8d 100644 --- a/server/modules/filter/namedserverfilter/namedserverfilter.cc +++ b/server/modules/filter/namedserverfilter/namedserverfilter.cc @@ -805,7 +805,7 @@ SourceHost* RegexHintFilter::set_source_address(const char* input_host) struct addrinfo *ai = NULL, hint = {}; hint.ai_flags = AI_ADDRCONFIG | AI_V4MAPPED; - int rc = getaddrinfo(input_host, NULL, &hint, &ai); + int rc = getaddrinfo(format_host, NULL, &hint, &ai); /* fill IPv4 data struct */ if (rc == 0) From 9799cfdb2b8ecd54b1c97b8f2182d35780065ee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 23 Aug 2018 08:57:58 +0300 Subject: [PATCH 09/17] MXS-1735: Clarify differences between MaxScale and server The server allows both unencrypted and encrypted connections on the same port. MaxScale only allows either encrypted or unencrypted connections. The differences as well as the reasoning for this need to be documented. --- .../Getting-Started/Configuration-Guide.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Documentation/Getting-Started/Configuration-Guide.md b/Documentation/Getting-Started/Configuration-Guide.md index 8e0abb874..22f0100d9 100644 --- a/Documentation/Getting-Started/Configuration-Guide.md +++ b/Documentation/Getting-Started/Configuration-Guide.md @@ -1390,8 +1390,20 @@ to `required` and provide the three files for `ssl_cert`, `ssl_key` and After this, MaxScale connections between the server and/or the client will be encrypted. Note that the database must be configured to use TLS/SSL connections -if backend connection encryption is used. When client-side encryption is -enabled, only encrypted connections to MaxScale can be created. +if backend connection encryption is used. + +**Note:** MaxScale does not allow mixed use of TLS/SSL and normal connections on + the same port. + +If TLS encryption is enabled for a listener, any unencrypted connections to it +will be rejected. MaxScale does this to improve security by preventing +accidental creation on unencrypted connections. + +The separation of secure and insecure connections differs from the MariaDB +server which allows both secure and insecure connections on the same port. As +MaxScale is the gateway through which all connections go, in order to guarantee +a more secure system MaxScale enforces a stricter security policy than what the +server does. #### `ssl` From 13c7072da1795fa62a89682aa6ab140c323d292a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 23 Aug 2018 23:52:42 +0300 Subject: [PATCH 10/17] Separate CCRFFilter hints section from the main overview The text changed subjects rather abruptly. Having the hint part of it gives some context and appears to make the text flow nicer. --- Documentation/Filters/CCRFilter.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/Filters/CCRFilter.md b/Documentation/Filters/CCRFilter.md index 3252c55d9..3028e1af3 100644 --- a/Documentation/Filters/CCRFilter.md +++ b/Documentation/Filters/CCRFilter.md @@ -12,6 +12,8 @@ a routing hint to all following statements. This routing hint guides the routing module to route the statement to the master server where data is guaranteed to be in an up-to-date state. +### Controlling the Filter with SQL Comments + The triggering of the filter can be limited further by adding MaxScale supported comments to queries and/or by using regular expressions. The query comments take precedence: if a comment is found it is obayed even if a regular expression From 2c54f28fae8acc4be471909c0afc7ae2f9a5a356 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 24 Aug 2018 20:52:52 +0300 Subject: [PATCH 11/17] MXS-2024: Validate COM_CHANGE_USER packet before use The use of strcpy on data that is assumed to be null terminated causes reads and writes past buffers. --- .../modules/protocol/MySQL/mariadbclient/mysql_client.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 692f80bbb..08bddce77 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -1545,6 +1546,14 @@ static bool reauthenticate_client(MXS_SESSION* session, GWBUF* packetbuf) gwbuf_copy_data(proto->stored_query, MYSQL_HEADER_LEN + 1, sizeof(user), (uint8_t*)user); + char* end = user + sizeof(user); + + if (std::find(user, end, '\0') == end) + { + mysql_send_auth_error(session->client_dcb, 3, 0, "Malformed AuthSwitchRequest packet"); + return false; + } + // Copy the new username to the session data MYSQL_session* data = (MYSQL_session*)session->client_dcb->data; strcpy(data->user, user); From 02a65f311aa61c5f17ade2a215097d3b489d9a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 27 Aug 2018 12:00:28 +0300 Subject: [PATCH 12/17] Fix memory leak in cdc-connector The Closer default value was wrong. --- connectors/cdc-connector/cdc_connector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index 00eb808dc..69ca1de4d 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -122,7 +122,7 @@ public: Closer(T t): m_t(t), - m_close(false) + m_close(true) { } From 01e1c616ba0b4de1533a193639f36b3e83d76efb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 27 Aug 2018 12:11:42 +0300 Subject: [PATCH 13/17] Fix CDC error detection The error detection assumed the buffer was null-terminated which was never guaranteed. --- connectors/cdc-connector/cdc_connector.cpp | 19 ++++++++++--------- connectors/cdc-connector/cdc_connector.h | 2 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index 69ca1de4d..ca2ac80a8 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -523,14 +523,14 @@ bool Connection::do_registration() return rval; } -bool Connection::is_error(const char* str) +bool Connection::is_error() { bool rval = false; - if (str[0] == 'E' && str[1] == 'R' && str[2] == 'R') + if (m_buffer.size() >= 3 && m_buffer[0] == 'E' && m_buffer[1] == 'R' && m_buffer[2] == 'R') { m_error = "MaxScale responded with an error: "; - m_error += str; + m_error.append(m_buffer.begin(), m_buffer.end()); rval = true; } @@ -545,6 +545,12 @@ bool Connection::read_row(std::string& dest) { if (!m_buffer.empty()) { + if (is_error()) + { + rval = false; + break; + } + std::deque::iterator it = std::find(m_buffer.begin(), m_buffer.end(), '\n'); if (it != m_buffer.end()) @@ -577,18 +583,13 @@ bool Connection::read_row(std::string& dest) assert(std::find(m_buffer.begin(), m_buffer.end(), '\n') == m_buffer.end()); std::copy(buf, buf + rc, std::back_inserter(m_buffer)); - if (is_error(&m_buffer[0])) + if (is_error()) { rval = false; break; } } - if (is_error(dest.c_str())) - { - rval = false; - } - return rval; } diff --git a/connectors/cdc-connector/cdc_connector.h b/connectors/cdc-connector/cdc_connector.h index e91c371f1..c5787d165 100644 --- a/connectors/cdc-connector/cdc_connector.h +++ b/connectors/cdc-connector/cdc_connector.h @@ -147,7 +147,7 @@ private: bool read_row(std::string& dest); void process_schema(json_t* json); SRow process_row(json_t*); - bool is_error(const char* str); + bool is_error(); // Lower-level functions int wait_for_event(short events); From 69f53f886dde3d2118136fe7c175d30ae2a1408f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 27 Aug 2018 12:31:02 +0300 Subject: [PATCH 14/17] Append received data to CDC::TIMEOUT errors The extra output of received data should be added only to timeout errors, not to all errors. --- connectors/cdc-connector/cdc_connector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index ca2ac80a8..802719277 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -280,7 +280,7 @@ bool Connection::connect(const std::string& table, const std::string& gtid) { rval = true; } - else + else if (m_error == CDC::TIMEOUT) { m_error += ". Data received so far: '"; std::copy(m_buffer.begin(), m_buffer.end(), std::back_inserter(m_error)); From 68b4f20436e7ac71708c604dad74c40dc4696f7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 27 Aug 2018 13:22:16 +0300 Subject: [PATCH 15/17] Split schema and row processing The recursive calls into `read` caused unnecessary slowness in the connection phase. The actual first row should only be read when the data is requested. This can possibly solve the false timeout errors caused by slow sending of the first row of data. --- connectors/cdc-connector/cdc_connector.cpp | 59 ++++++++++++++-------- connectors/cdc-connector/cdc_connector.h | 1 + 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/connectors/cdc-connector/cdc_connector.cpp b/connectors/cdc-connector/cdc_connector.cpp index 802719277..f5913bf00 100644 --- a/connectors/cdc-connector/cdc_connector.cpp +++ b/connectors/cdc-connector/cdc_connector.cpp @@ -276,16 +276,10 @@ bool Connection::connect(const std::string& table, const std::string& gtid) m_error = "Failed to write request: "; m_error += strerror_r(errno, err, sizeof(err)); } - else if ((m_first_row = read())) + else if (read_schema()) { rval = true; } - else if (m_error == CDC::TIMEOUT) - { - m_error += ". Data received so far: '"; - std::copy(m_buffer.begin(), m_buffer.end(), std::back_inserter(m_error)); - m_error += "'"; - } } } } @@ -397,18 +391,13 @@ SRow Connection::process_row(json_t* js) return rval; } -SRow Connection::read() +bool Connection::read_schema() { m_error.clear(); - SRow rval; + bool rval = false; std::string row; - if (m_first_row) - { - rval.swap(m_first_row); - assert(!m_first_row); - } - else if (read_row(row)) + if (read_row(row)) { json_error_t err; json_t* js = json_loads(row.c_str(), JSON_ALLOW_NUL, &err); @@ -419,11 +408,7 @@ SRow Connection::read() { m_schema = row; process_schema(js); - rval = Connection::read(); - } - else - { - rval = process_row(js); + rval = true; } json_decref(js); @@ -435,6 +420,40 @@ SRow Connection::read() } } + if (m_error == CDC::TIMEOUT) + { + assert(rval == false); + m_error += ". Data received so far: '"; + std::copy(m_buffer.begin(), m_buffer.end(), std::back_inserter(m_error)); + m_error += "'"; + } + + return rval; +} + +SRow Connection::read() +{ + m_error.clear(); + SRow rval; + std::string row; + + if (read_row(row)) + { + json_error_t err; + json_t* js = json_loads(row.c_str(), JSON_ALLOW_NUL, &err); + + if (js) + { + rval = process_row(js); + json_decref(js); + } + else + { + m_error = "Failed to parse JSON: "; + m_error += err.text; + } + } + return rval; } diff --git a/connectors/cdc-connector/cdc_connector.h b/connectors/cdc-connector/cdc_connector.h index c5787d165..fa0b8e40f 100644 --- a/connectors/cdc-connector/cdc_connector.h +++ b/connectors/cdc-connector/cdc_connector.h @@ -145,6 +145,7 @@ private: bool do_auth(); bool do_registration(); bool read_row(std::string& dest); + bool read_schema(); void process_schema(json_t* json); SRow process_row(json_t*); bool is_error(); From a50e8e9ce6c367f8477bd749f3e0a49b6bdaefd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 27 Aug 2018 20:35:09 +0300 Subject: [PATCH 16/17] MXS-2024: Prevent stack overflow If a large packet is received, the stack would overflow when the username size was determined from the packet size. The code must not assume anything about the size of the packet being read. --- .../protocol/MySQL/mariadbclient/mysql_client.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 08bddce77..136eb4ac8 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -1537,14 +1538,15 @@ static bool reauthenticate_client(MXS_SESSION* session, GWBUF* packetbuf) if (session->client_dcb->authfunc.reauthenticate) { + uint64_t payloadlen = gwbuf_length(packetbuf) - MYSQL_HEADER_LEN; MySQLProtocol* proto = (MySQLProtocol*)session->client_dcb->protocol; - uint8_t payload[gwbuf_length(packetbuf) - MYSQL_HEADER_LEN]; - gwbuf_copy_data(packetbuf, MYSQL_HEADER_LEN, sizeof(payload), payload); + std::vector payload; + payload.resize(payloadlen); + gwbuf_copy_data(packetbuf, MYSQL_HEADER_LEN, payloadlen, &payload[0]); // Will contains extra data but the username is null-terminated - char user[gwbuf_length(proto->stored_query) - MYSQL_HEADER_LEN - 1]; - gwbuf_copy_data(proto->stored_query, MYSQL_HEADER_LEN + 1, - sizeof(user), (uint8_t*)user); + char user[MYSQL_USER_MAXLEN + 1]; + gwbuf_copy_data(proto->stored_query, MYSQL_HEADER_LEN + 1, sizeof(user), (uint8_t*)user); char* end = user + sizeof(user); @@ -1559,7 +1561,7 @@ static bool reauthenticate_client(MXS_SESSION* session, GWBUF* packetbuf) strcpy(data->user, user); int rc = session->client_dcb->authfunc.reauthenticate(session->client_dcb, data->user, - payload, sizeof(payload), + &payload[0], payload.size(), proto->scramble, sizeof(proto->scramble), data->client_sha1, sizeof(data->client_sha1)); From 51ade9e6fe16fce4153a77d8dd1cd91848478151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 28 Aug 2018 08:39:38 +0300 Subject: [PATCH 17/17] Wait two monitor intervals in failover tests Since the failover always takes two intervals, the test also needs to wait two intervals. --- maxscale-system-test/failover_common.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/maxscale-system-test/failover_common.cpp b/maxscale-system-test/failover_common.cpp index 9377e117a..30d4c46fe 100644 --- a/maxscale-system-test/failover_common.cpp +++ b/maxscale-system-test/failover_common.cpp @@ -24,13 +24,13 @@ void reset_replication(TestConnections& test) { int ind = master_id - 1; replicate_from(test, 0, ind); - test.maxscales->wait_for_monitor(); + test.maxscales->wait_for_monitor(2); get_output(test); int ec; stringstream switchover; switchover << "maxadmin call command mysqlmon switchover MySQL-Monitor server1 server" << master_id; test.maxscales->ssh_node_output(0, switchover.str().c_str() , true, &ec); - test.maxscales->wait_for_monitor(); + test.maxscales->wait_for_monitor(2); master_id = get_master_server_id(test); cout << "Master server id is now back to " << master_id << endl; test.assert(master_id == 1, "Switchover back to server1 failed"); @@ -106,7 +106,7 @@ void check_test_2(TestConnections& test) // Reset state replicate_from(test, 1, master_id - 1); - test.maxscales->wait_for_monitor(); + test.maxscales->wait_for_monitor(2); get_output(test); StringSet node_states = test.get_server_status("server2"); test.assert(node_states.find("Slave") != node_states.end(), "Server 2 is not replicating."); @@ -143,7 +143,7 @@ void prepare_test_3(TestConnections& test) test.repl->start_node(2, (char *) ""); test.repl->start_node(3, (char *) ""); test.maxscales->start_maxscale(0); - test.maxscales->wait_for_monitor(); + test.maxscales->wait_for_monitor(2); test.repl->connect(); test.tprintf("Settings changed.");