From 8eaa265168f5529ad16aa965f381a69f8db87cec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 18 Jun 2018 20:42:23 +0300 Subject: [PATCH] MXS-1931: Remove use of gw_MySQL_get_next_packet The function implemented redundant functionality and replacement with modutil_get_next_MySQL_packet was planned. When faced with a packet header spread over multiple buffers, the packet length calculation would read past the buffer end. This is fixed by taking modutil_get_next_MySQL_packet into use. Identical behavior to the old function is achieved by calling gwbuf_make_contiguous for each packet to store them in a contiguous area of memory. This should be either removed and only done when RCAP_TYPE_CONTIGUOUS_INPUT is requested or be made an innate feature of statement based routing. --- include/maxscale/protocol/mysql.h | 2 - .../MySQL/mariadbclient/mysql_client.cc | 42 ++++----- server/modules/protocol/MySQL/mysql_common.cc | 94 ------------------- 3 files changed, 18 insertions(+), 120 deletions(-) diff --git a/include/maxscale/protocol/mysql.h b/include/maxscale/protocol/mysql.h index 88f0b0bc7..dd8369b00 100644 --- a/include/maxscale/protocol/mysql.h +++ b/include/maxscale/protocol/mysql.h @@ -464,8 +464,6 @@ int mysql_send_custom_error(DCB *dcb, int sequence, int affected_rows, const cha int mysql_send_standard_error(DCB *dcb, int sequence, int errnum, const char *msg); int mysql_send_auth_error(DCB *dcb, int sequence, int affected_rows, const char* msg); -GWBUF* gw_MySQL_get_next_packet(GWBUF** p_readbuf); -GWBUF* gw_MySQL_get_packets(GWBUF** p_readbuf, int* npackets); void protocol_add_srv_command(MySQLProtocol* p, mxs_mysql_cmd_t cmd); void protocol_remove_srv_command(MySQLProtocol* p); bool protocol_waits_response(MySQLProtocol* p); diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index a7ce5c300..22db11462 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -1576,6 +1576,17 @@ static bool reauthenticate_client(MXS_SESSION* session, GWBUF* packetbuf) return rval; } +// Helper function for debug assertions +static bool only_one_packet(GWBUF* buffer) +{ + ss_dassert(buffer); + uint8_t header[4] = {}; + gwbuf_copy_data(buffer, 0, MYSQL_HEADER_LEN, header); + size_t packet_len = gw_mysql_get_byte3(header); + size_t buffer_len = gwbuf_length(buffer); + return packet_len + MYSQL_HEADER_LEN == buffer_len; +} + /** * Detect if buffer includes partial mysql packet or multiple packets. * Store partial packet to dcb_readqueue. Send complete packets one by one @@ -1597,15 +1608,15 @@ static int route_by_statement(MXS_SESSION* session, uint64_t capabilities, GWBUF GWBUF* packetbuf; do { - /** - * Collect incoming bytes to a buffer until complete packet has - * arrived and then return the buffer. - */ - // TODO: This should be replaced with modutil_get_next_MySQL_packet. - packetbuf = gw_MySQL_get_next_packet(p_readbuf); + // Process client request one packet at a time + packetbuf = modutil_get_next_MySQL_packet(p_readbuf); + + // TODO: Do this only when RCAP_TYPE_CONTIGUOUS_INPUT is requested + packetbuf = gwbuf_make_contiguous(packetbuf); if (packetbuf != NULL) { + ss_dassert(only_one_packet(packetbuf)); CHK_GWBUF(packetbuf); MySQLProtocol* proto = (MySQLProtocol*)session->client_dcb->protocol; @@ -1619,24 +1630,7 @@ static int route_by_statement(MXS_SESSION* session, uint64_t capabilities, GWBUF if (rcap_type_required(capabilities, RCAP_TYPE_CONTIGUOUS_INPUT)) { - if (!GWBUF_IS_CONTIGUOUS(packetbuf)) - { - // TODO: As long as gw_MySQL_get_next_packet is used above, the buffer - // TODO: will be contiguous. That function should be replaced with - // TODO: modutil_get_next_MySQL_packet. - GWBUF* tmp = gwbuf_make_contiguous(packetbuf); - if (tmp) - { - packetbuf = tmp; - } - else - { - // TODO: A memory allocation failure. We should close the dcb - // TODO: and terminate the session. - rc = 0; - goto return_rc; - } - } + ss_dassert(GWBUF_IS_CONTIGUOUS(packetbuf)); if (rcap_type_required(capabilities, RCAP_TYPE_TRANSACTION_TRACKING)) { diff --git a/server/modules/protocol/MySQL/mysql_common.cc b/server/modules/protocol/MySQL/mysql_common.cc index 45609763c..d1b396504 100644 --- a/server/modules/protocol/MySQL/mysql_common.cc +++ b/server/modules/protocol/MySQL/mysql_common.cc @@ -473,100 +473,6 @@ int mysql_send_auth_error(DCB *dcb, return sizeof(mysql_packet_header) + mysql_payload_size; } - -/** - * Buffer contains at least one of the following: - * complete [complete] [partial] mysql packet - * - * @param p_readbuf Address of read buffer pointer - * - * @return pointer to gwbuf containing a complete packet or - * NULL if no complete packet was found. - */ -GWBUF* gw_MySQL_get_next_packet(GWBUF** p_readbuf) -{ - GWBUF* packetbuf; - GWBUF* readbuf; - size_t buflen; - size_t packetlen; - size_t totalbuflen; - uint8_t* data; - size_t nbytes_copied = 0; - uint8_t* target; - - readbuf = *p_readbuf; - - if (readbuf == NULL) - { - packetbuf = NULL; - goto return_packetbuf; - } - CHK_GWBUF(readbuf); - - if (GWBUF_EMPTY(readbuf)) - { - packetbuf = NULL; - goto return_packetbuf; - } - totalbuflen = gwbuf_length(readbuf); - data = (uint8_t *)GWBUF_DATA((readbuf)); - packetlen = MYSQL_GET_PAYLOAD_LEN(data) + 4; - - /** packet is incomplete */ - if (packetlen > totalbuflen) - { - packetbuf = NULL; - goto return_packetbuf; - } - - packetbuf = gwbuf_alloc(packetlen); - target = GWBUF_DATA(packetbuf); - packetbuf->gwbuf_type = readbuf->gwbuf_type; /*< Copy the type too */ - /** - * Copy first MySQL packet to packetbuf and leave posible other - * packets to read buffer. - */ - while (nbytes_copied < packetlen && totalbuflen > 0) - { - uint8_t* src = GWBUF_DATA((*p_readbuf)); - size_t bytestocopy; - - buflen = GWBUF_LENGTH((*p_readbuf)); - bytestocopy = buflen < (packetlen - nbytes_copied) ? buflen : packetlen - nbytes_copied; - - memcpy(target + nbytes_copied, src, bytestocopy); - *p_readbuf = gwbuf_consume((*p_readbuf), bytestocopy); - totalbuflen = gwbuf_length((*p_readbuf)); - nbytes_copied += bytestocopy; - } - ss_dassert(buflen == 0 || nbytes_copied == packetlen); - -return_packetbuf: - return packetbuf; -} - -/** - * Move from buffer pointed to by <*p_readbuf>. - * Appears to be unused 11 May 2016 (Martin) - */ -GWBUF* gw_MySQL_get_packets(GWBUF** p_srcbuf, - int* npackets) -{ - GWBUF* packetbuf; - GWBUF* targetbuf = NULL; - - while (*npackets > 0 && (packetbuf = gw_MySQL_get_next_packet(p_srcbuf)) != NULL) - { - targetbuf = gwbuf_append(targetbuf, packetbuf); - *npackets -= 1; - } - ss_dassert(*npackets < 128); - ss_dassert(*npackets >= 0); - - return targetbuf; -} - - static server_command_t* server_command_init(server_command_t* srvcmd, mxs_mysql_cmd_t cmd) {