From 92ef33327edbd95f3908850f31ed777ce225b429 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 19 Sep 2016 05:34:03 +0300 Subject: [PATCH] MXS-870: Handle non-contiguous session command responses Session command responses with multiple packets could be spread across multiple, non-contiguous buffers. If a buffer contained a complete packet and some extra data but it wasn't contiguous, the debug assertion in gwbuf_clone_portion would fail. With release builds, it would cause eventual out-of-bounds memory access when the response would be sent to the client. --- server/core/buffer.c | 6 +++--- server/core/test/testbuffer.c | 9 --------- server/include/buffer.h | 1 - server/modules/protocol/mysql_backend.c | 23 +++++++++-------------- 4 files changed, 12 insertions(+), 27 deletions(-) diff --git a/server/core/buffer.c b/server/core/buffer.c index ce4b794b9..b2dfa4c72 100644 --- a/server/core/buffer.c +++ b/server/core/buffer.c @@ -384,9 +384,9 @@ GWBUF* gwbuf_clone_all(GWBUF* buf) } -GWBUF *gwbuf_clone_portion(GWBUF *buf, - size_t start_offset, - size_t length) +static GWBUF *gwbuf_clone_portion(GWBUF *buf, + size_t start_offset, + size_t length) { GWBUF* clonebuf; diff --git a/server/core/test/testbuffer.c b/server/core/test/testbuffer.c index 8ec09d6d3..ad26f0a5d 100644 --- a/server/core/test/testbuffer.c +++ b/server/core/test/testbuffer.c @@ -375,15 +375,6 @@ test1() gwbuf_free(clone); ss_dfprintf(stderr, "Freed cloned buffer"); ss_dfprintf(stderr, "\t..done\n"); - partclone = gwbuf_clone_portion(buffer, 25, 50); - buflen = GWBUF_LENGTH(partclone); - ss_dfprintf(stderr, "Part cloned buffer length is now %d", buflen); - ss_info_dassert(50 == buflen, "Incorrect buffer size"); - ss_info_dassert(0 == GWBUF_EMPTY(partclone), "Part cloned buffer should not be empty"); - ss_dfprintf(stderr, "\t..done\n"); - gwbuf_free(partclone); - ss_dfprintf(stderr, "Freed part cloned buffer"); - ss_dfprintf(stderr, "\t..done\n"); buffer = gwbuf_consume(buffer, bite1); ss_info_dassert(NULL != buffer, "Buffer should not be null"); buflen = GWBUF_LENGTH(buffer); diff --git a/server/include/buffer.h b/server/include/buffer.h index 42d0aa016..2a55f4963 100644 --- a/server/include/buffer.h +++ b/server/include/buffer.h @@ -194,7 +194,6 @@ extern unsigned int gwbuf_length(GWBUF *head); extern int gwbuf_count(GWBUF *head); extern size_t gwbuf_copy_data(GWBUF *buffer, size_t offset, size_t bytes, uint8_t* dest); -extern GWBUF *gwbuf_clone_portion(GWBUF *head, size_t offset, size_t len); extern GWBUF *gwbuf_split(GWBUF **buf, size_t length); extern GWBUF *gwbuf_clone_transform(GWBUF *head, gwbuf_type_t type); extern GWBUF *gwbuf_clone_all(GWBUF* head); diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 5addc27b8..2a9bbbb7d 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -2011,22 +2011,17 @@ static GWBUF* process_response_data(DCB* dcb, outbuf = gwbuf_append(outbuf, readbuf); readbuf = NULL; } - /** - * Packet was read. There should be more since bytes were - * left over. - * Move the next packet to its own buffer and add that next - * to the prev packet's buffer. - */ - else /*< nbytes_left < nbytes_to_process */ + /** + * Buffer contains more data than we need. Split the complete packet and + * the extra data into two separate buffers. + */ + else { - ss_dassert(nbytes_left >= 0); - nbytes_to_process -= nbytes_left; - - /** Move the prefix of the buffer to outbuf from redbuf */ - outbuf = gwbuf_append(outbuf, - gwbuf_clone_portion(readbuf, 0, (size_t) nbytes_left)); - readbuf = gwbuf_consume(readbuf, (size_t) nbytes_left); + ss_dassert(nbytes_left < nbytes_to_process); + ss_dassert(nbytes_left > 0); ss_dassert(npackets_left > 0); + outbuf = gwbuf_append(outbuf, gwbuf_split(&readbuf, nbytes_left)); + nbytes_to_process -= nbytes_left; npackets_left -= 1; nbytes_left = 0; }