MXS-2470 Validate GWBUFs

A GWBUF given to any gwbuf-function:

- Must not be NULL. Exceptions are gwbuf_free() and gwbuf_append(),
  in analogy with free() and realloc() respectively.
- Must be the head of a chain.
- Must be owned by the calling thread.

include/maxscale/buffer.hh

@@ -197,7 +197,7 @@ extern GWBUF* gwbuf_alloc_and_load(unsigned int size, const void* data);
 /**
  * Free a chain of gateway buffers
  *
- * @param buf  The head of the list of buffers to free
+ * @param buf  The head of the list of buffers to free, can be NULL.
  */
 extern void gwbuf_free(GWBUF* buf);
 
@@ -208,8 +208,7 @@ extern void gwbuf_free(GWBUF* buf);
  *
  * @param buf  The GWBUF to be cloned.
  *
- * @return The cloned GWBUF, or NULL if @buf was NULL or if any part
+ * @return The cloned GWBUF, or NULL if any part of @buf could not be cloned.
- *         of @buf could not be cloned.
  */
 extern GWBUF* gwbuf_clone(GWBUF* buf);
 
@@ -240,9 +239,8 @@ extern GWBUF* gwbuf_deep_clone(const GWBUF* buf);
  *         -1 if @c lhs is less than @c rhs, and
  *          1 if @c lhs is more than @c rhs.
  *
- * @attention A NULL @c GWBUF is considered to be less than a non-NULL one,
+ * @attention A shorter @c GWBUF less than a longer one. Otherwise the
- *            and a shorter @c GWBUF less than a longer one. Otherwise the
+ *            sign of the return value is determined by the sign of the
- *            the sign of the return value is determined by the sign of the
  *            difference between the first pair of bytes (interpreted as
  *            unsigned char) that differ in lhs and rhs.
  */
@@ -251,11 +249,8 @@ extern int gwbuf_compare(const GWBUF* lhs, const GWBUF* rhs);
 /**
  * Append a buffer onto a linked list of buffer structures.
  *
- * This call should be made with the caller holding the lock for the linked
+ * @param head  The current head of the linked list or NULL.
- * list.
+ * @param tail  Another buffer to make the tail of the linked list
- *
- * @param head  The current head of the linked list
- * @param tail  The new buffer to make the tail of the linked list
  *
  * @return The new head of the linked list
  */

server/core/buffer.cc

@@ -42,8 +42,42 @@ inline void invalidate_tail_pointers(GWBUF* head)
         }
     }
 }
+
+inline void ensure_at_head(const GWBUF* buf)
+{
+    mxb_assert(buf->tail != reinterpret_cast<GWBUF*>(0xdeadbeef));
+}
+
+inline void ensure_owned(const GWBUF* buf)
+{
+    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+}
+
+inline bool validate_buffer(const GWBUF* buf)
+{
+    mxb_assert(buf);
+    ensure_at_head(buf);
+    ensure_owned(buf);
+    return true;
+}
+
 #else
-inline void invalidate_tail_pointers(GWBUF* head) {}
+inline void invalidate_tail_pointers(GWBUF* head)
+{
+}
+
+inline void ensure_at_head(const GWBUF* head)
+{
+}
+
+inline void ensure_owned(const GWBUF* head)
+{
+}
+
+inline bool validate_buffer(const GWBUF* head)
+{
+    return true;
+}
 #endif
 
 /**
@@ -117,9 +151,10 @@ GWBUF* gwbuf_alloc_and_load(unsigned int size, const void* data)
  */
 void gwbuf_free(GWBUF* buf)
 {
+    mxb_assert(!buf || validate_buffer(buf));
+
     while (buf)
     {
-        mxb_assert(buf->owner == RoutingWorker::get_current_id());
         GWBUF* nextbuf = buf->next;
         gwbuf_free_one(buf);
         buf = nextbuf;
@@ -133,6 +168,8 @@ void gwbuf_free(GWBUF* buf)
  */
 static void gwbuf_free_one(GWBUF* buf)
 {
+    ensure_owned(buf);
+
     --buf->sbuf->refcount;
 
     if (buf->sbuf->refcount == 0)
@@ -204,14 +241,8 @@ static GWBUF* gwbuf_clone_one(GWBUF* buf)
 
 GWBUF* gwbuf_clone(GWBUF* buf)
 {
-    mxb_assert(buf);
+    validate_buffer(buf);
 
-    if (!buf)
-    {
-        return NULL;
-    }
-
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
     GWBUF* rval = gwbuf_clone_one(buf);
 
     if (rval)
@@ -244,7 +275,7 @@ GWBUF* gwbuf_clone(GWBUF* buf)
 
 static GWBUF* gwbuf_deep_clone_portion(const GWBUF* buf, size_t length)
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    ensure_owned(buf);
     GWBUF* rval = NULL;
 
     if (buf)
@@ -269,6 +300,7 @@ static GWBUF* gwbuf_deep_clone_portion(const GWBUF* buf, size_t length)
 
 GWBUF* gwbuf_deep_clone(const GWBUF* buf)
 {
+    validate_buffer(buf);
     return gwbuf_deep_clone_portion(buf, gwbuf_length(buf));
 }
 
@@ -276,7 +308,7 @@ static GWBUF* gwbuf_clone_portion(GWBUF* buf,
                                   size_t start_offset,
                                   size_t length)
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    ensure_owned(buf);
     mxb_assert(start_offset + length <= GWBUF_LENGTH(buf));
 
     GWBUF* clonebuf = (GWBUF*)MXS_MALLOC(sizeof(GWBUF));
@@ -306,6 +338,7 @@ static GWBUF* gwbuf_clone_portion(GWBUF* buf,
 
 GWBUF* gwbuf_split(GWBUF** buf, size_t length)
 {
+    validate_buffer(*buf);
     GWBUF* head = NULL;
 
     if (length > 0 && buf && *buf)
@@ -313,7 +346,7 @@ GWBUF* gwbuf_split(GWBUF** buf, size_t length)
         GWBUF* buffer = *buf;
         GWBUF* orig_tail = buffer->tail;
         head = buffer;
-        mxb_assert(buffer->owner == RoutingWorker::get_current_id());
+        ensure_owned(buffer);
 
         /** Handle complete buffers */
         while (buffer && length && length >= GWBUF_LENGTH(buffer))
@@ -409,27 +442,10 @@ static inline bool gwbuf_get_byte(const GWBUF** buf, size_t* offset, uint8_t* b)
 
 int gwbuf_compare(const GWBUF* lhs, const GWBUF* rhs)
 {
-    int rv;
+    validate_buffer(lhs);
+    validate_buffer(rhs);
 
-    if ((lhs == NULL) && (rhs == NULL))
+    int rv;
-    {
-        rv = 0;
-    }
-    else if (lhs == NULL)
-    {
-        mxb_assert(rhs);
-        rv = -1;
-    }
-    else if (rhs == NULL)
-    {
-        mxb_assert(lhs);
-        rv = 1;
-    }
-    else
-    {
-        mxb_assert(lhs->owner == RoutingWorker::get_current_id()
-                   && rhs->owner == RoutingWorker::get_current_id());
-        mxb_assert(lhs && rhs);
 
     size_t llen = gwbuf_length(lhs);
     size_t rlen = gwbuf_length(rhs);
@@ -475,24 +491,19 @@ int gwbuf_compare(const GWBUF* lhs, const GWBUF* rhs)
             rv = 1;
         }
     }
-    }
 
     return rv;
 }
 
 GWBUF* gwbuf_append(GWBUF* head, GWBUF* tail)
 {
-    mxb_assert(!head || head->owner == RoutingWorker::get_current_id());
+    mxb_assert(!head || validate_buffer(head));
-    mxb_assert(!tail || tail->owner == RoutingWorker::get_current_id());
+    mxb_assert(validate_buffer(tail));
 
     if (!head)
     {
         return tail;
     }
-    else if (!tail)
-    {
-        return head;
-    }
 
     head->tail->next = tail;
     head->tail = tail->tail;
@@ -504,6 +515,8 @@ GWBUF* gwbuf_append(GWBUF* head, GWBUF* tail)
 
 GWBUF* gwbuf_consume(GWBUF* head, unsigned int length)
 {
+    validate_buffer(head);
+
     while (head && length > 0)
     {
         mxb_assert(head->owner == RoutingWorker::get_current_id());
@@ -532,11 +545,13 @@ GWBUF* gwbuf_consume(GWBUF* head, unsigned int length)
 
 unsigned int gwbuf_length(const GWBUF* head)
 {
+    validate_buffer(head);
+
     int rval = 0;
 
     while (head)
     {
-        mxb_assert(head->owner == RoutingWorker::get_current_id());
+        ensure_owned(head);
         rval += GWBUF_LENGTH(head);
         head = head->next;
     }
@@ -546,11 +561,13 @@ unsigned int gwbuf_length(const GWBUF* head)
 
 int gwbuf_count(const GWBUF* head)
 {
+    validate_buffer(head);
+
     int result = 0;
 
     while (head)
     {
-        mxb_assert(head->owner == RoutingWorker::get_current_id());
+        ensure_owned(head);
         result++;
         head = head->next;
     }
@@ -560,7 +577,8 @@ int gwbuf_count(const GWBUF* head)
 
 GWBUF* gwbuf_rtrim(GWBUF* head, unsigned int n_bytes)
 {
-    mxb_assert(head->owner == RoutingWorker::get_current_id());
+    validate_buffer(head);
+
     GWBUF* rval = head;
     GWBUF_RTRIM(head, n_bytes);
 
@@ -574,6 +592,7 @@ GWBUF* gwbuf_rtrim(GWBUF* head, unsigned int n_bytes)
 
 void gwbuf_set_type(GWBUF* buf, uint32_t type)
 {
+    validate_buffer(buf);
     /** Set type consistenly to all buffers on the list */
     while (buf != NULL)
     {
@@ -588,7 +607,8 @@ void gwbuf_add_buffer_object(GWBUF* buf,
                              void* data,
                              void (* donefun_fp)(void*))
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    validate_buffer(buf);
+
     buffer_object_t* newb = (buffer_object_t*)MXS_MALLOC(sizeof(buffer_object_t));
     MXS_ABORT_IF_NULL(newb);
 
@@ -610,7 +630,8 @@ void gwbuf_add_buffer_object(GWBUF* buf,
 
 void* gwbuf_get_buffer_object_data(GWBUF* buf, bufobj_id_t id)
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    validate_buffer(buf);
+
     buffer_object_t* bo = buf->sbuf->bufobj;
 
     while (bo != NULL && bo->bo_id != id)
@@ -626,7 +647,7 @@ void* gwbuf_get_buffer_object_data(GWBUF* buf, bufobj_id_t id)
  */
 static buffer_object_t* gwbuf_remove_buffer_object(GWBUF* buf, buffer_object_t* bufobj)
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    ensure_owned(buf);
     buffer_object_t* next = bufobj->bo_next;
     /** Call corresponding clean-up function to clean buffer object's data */
     bufobj->bo_donefun_fp(bufobj->bo_data);
@@ -636,7 +657,8 @@ static buffer_object_t* gwbuf_remove_buffer_object(GWBUF* buf, buffer_object_t*
 
 bool gwbuf_add_property(GWBUF* buf, const char* name, const char* value)
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    validate_buffer(buf);
+
     char* my_name = MXS_STRDUP(name);
     char* my_value = MXS_STRDUP(value);
     BUF_PROPERTY* prop = (BUF_PROPERTY*)MXS_MALLOC(sizeof(BUF_PROPERTY));
@@ -659,7 +681,8 @@ bool gwbuf_add_property(GWBUF* buf, const char* name, const char* value)
 
 char* gwbuf_get_property(GWBUF* buf, const char* name)
 {
-    mxb_assert(buf->owner == RoutingWorker::get_current_id());
+    validate_buffer(buf);
+
     BUF_PROPERTY* prop = buf->properties;
 
     while (prop && strcmp(prop->name, name) != 0)
@@ -672,8 +695,7 @@ char* gwbuf_get_property(GWBUF* buf, const char* name)
 
 GWBUF* gwbuf_make_contiguous(GWBUF* orig)
 {
-    mxb_assert_message(orig != NULL, "gwbuf_make_contiguous: NULL buffer");
+    validate_buffer(orig);
-    mxb_assert(orig->owner == RoutingWorker::get_current_id());
 
     if (orig->next == NULL)
     {
@@ -701,6 +723,7 @@ GWBUF* gwbuf_make_contiguous(GWBUF* orig)
 
 size_t gwbuf_copy_data(const GWBUF* buffer, size_t offset, size_t bytes, uint8_t* dest)
 {
+    validate_buffer(buffer);
     uint32_t buflen;
 
     /** Skip unrelated buffers */
@@ -751,6 +774,7 @@ size_t gwbuf_copy_data(const GWBUF* buffer, size_t offset, size_t bytes, uint8_t
 
 uint8_t* gwbuf_byte_pointer(GWBUF* buffer, size_t offset)
 {
+    validate_buffer(buffer);
     uint8_t* rval = NULL;
     // Ignore NULL buffer and walk past empty or too short buffers.
     while (buffer && (GWBUF_LENGTH(buffer) <= offset))
@@ -771,7 +795,7 @@ uint8_t* gwbuf_byte_pointer(GWBUF* buffer, size_t offset)
 
 static std::string dump_one_buffer(GWBUF* buffer)
 {
-    mxb_assert(buffer->owner == RoutingWorker::get_current_id());
+    ensure_owned(buffer);
     std::string rval;
     int len = GWBUF_LENGTH(buffer);
     uint8_t* data = GWBUF_DATA(buffer);
@@ -800,6 +824,7 @@ static std::string dump_one_buffer(GWBUF* buffer)
 
 void gwbuf_hexdump(GWBUF* buffer, int log_level)
 {
+    validate_buffer(buffer);
     mxb_assert(buffer->owner == RoutingWorker::get_current_id());
     std::stringstream ss;
 

server/core/modutil.cc

@@ -556,6 +556,8 @@ static size_t get_complete_packets_length(GWBUF* buffer)
     size_t offset = 0;
     size_t total = 0;
 
+    GWBUF* tail = buffer ? buffer->tail : nullptr;
+
     while (buffer && gwbuf_copy_data(buffer, offset, 3, packet_len) == 3)
     {
         uint32_t len = gw_mysql_get_byte3(packet_len) + MYSQL_HEADER_LEN;
@@ -579,6 +581,13 @@ static size_t get_complete_packets_length(GWBUF* buffer)
                 buflen = buffer ? GWBUF_LENGTH(buffer) : 0;
             }
 
+            // TODO: Fix GWBUF interface so that this function can be written without
+            // TODO: knowledge about the internals of GWBUF.
+            if (buffer)
+            {
+                buffer->tail = tail;
+            }
+
             /** Either the buffer ended with a complete packet or the buffer
              * contains more data than is required. */
             if (read_len == 0 || (buffer && read_len < buflen))

server/core/test/test_buffer.cc

@@ -199,10 +199,6 @@ void test_split()
     gwbuf_free(newbuf);
 
     /** Bad parameter tests */
-    GWBUF* ptr = NULL;
-    mxb_assert_message(gwbuf_split(NULL, 0) == NULL, "gwbuf_split with NULL parameter should return NULL");
-    mxb_assert_message(gwbuf_split(&ptr, 0) == NULL,
-                       "gwbuf_split with pointer to a NULL value should return NULL");
     buffer = gwbuf_alloc(10);
     mxb_assert_message(gwbuf_split(&buffer, 0) == NULL, "gwbuf_split with length of 0 should return NULL");
     mxb_assert_message(gwbuf_length(buffer) == 10, "Buffer should be 10 bytes");
@@ -330,23 +326,13 @@ void test_compare()
 
     fprintf(stderr, "testbuffer : testing GWBUF comparisons\n");
 
-    GWBUF* lhs = NULL;
+    GWBUF* lhs = gwbuf_alloc_and_load(10, data);
-    GWBUF* rhs = NULL;
-
-    // Both NULL
-    mxb_assert(gwbuf_compare(lhs, rhs) == 0);
-
-    // Either (but not both) NULL
-    lhs = gwbuf_alloc_and_load(10, data);
-    mxb_assert(gwbuf_compare(lhs, rhs) > 0);
-    mxb_assert(gwbuf_compare(rhs, lhs) < 0);
 
     // The same array
     mxb_assert(gwbuf_compare(lhs, lhs) == 0);
 
     // Identical array
-    gwbuf_free(rhs);
+    GWBUF* rhs = gwbuf_alloc_and_load(10, data);
-    rhs = gwbuf_alloc_and_load(10, data);
     mxb_assert(gwbuf_compare(lhs, rhs) == 0);
 
     // One shorter

server/core/test/test_modutil.cc

@@ -33,6 +33,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include <maxbase/log.hh>
 #include <maxscale/alloc.h>
 #include <maxscale/modutil.hh>
 #include <maxscale/buffer.hh>
@@ -227,13 +228,16 @@ void test_multiple_sql_packets1()
         tail = gwbuf_alloc_and_load(sizeof(resultset) - i, resultset + i);
         head = gwbuf_append(head, tail);
         complete = modutil_get_complete_packets(&head);
-        int headlen = gwbuf_length(head);
+        int headlen = head ? gwbuf_length(head) : 0;
         int completelen = complete ? gwbuf_length(complete) : 0;
         mxb_assert_message(headlen + completelen == sizeof(resultset),
                            "Both buffers should sum up to sizeof(resutlset) bytes");
         uint8_t databuf[sizeof(resultset)];
         gwbuf_copy_data(complete, 0, completelen, databuf);
+        if (head)
+        {
             gwbuf_copy_data(head, 0, headlen, databuf + completelen);
+        }
         mxb_assert_message(memcmp(databuf, resultset, sizeof(resultset)) == 0, "Data should be OK");
         gwbuf_free(head);
         gwbuf_free(complete);
@@ -259,13 +263,16 @@ void test_multiple_sql_packets1()
     mxb_assert_message(gwbuf_length(complete) == sizeof(resultset),
                        "Complete should be sizeof(resulset) bytes long");
 
-    unsigned int headlen = gwbuf_length(head);
+    unsigned int headlen = head ? gwbuf_length(head) : 0;
     unsigned int completelen = complete ? gwbuf_length(complete) : 0;
     uint8_t databuf[sizeof(resultset)];
     mxb_assert_message(gwbuf_copy_data(complete, 0, completelen, databuf) == completelen,
                        "Expected data should be readable");
+    if (head)
+    {
         mxb_assert_message(gwbuf_copy_data(head, 0, headlen, databuf + completelen) == headlen,
                            "Expected data should be readable");
+    }
     mxb_assert_message(memcmp(databuf, resultset, sizeof(resultset)) == 0, "Data should be OK");
 
     /** Fragmented buffer split into multiple chains and then reassembled as a complete resultset */
@@ -635,6 +642,8 @@ void test_bypass_whitespace()
 
 int main(int argc, char** argv)
 {
+    mxb::Log log;
+
     int result = 0;
 
     result += test1();