From 6e6a3e46264f95b0ab9b2b087c57c3299eec1e87 Mon Sep 17 00:00:00 2001
From: Johan Wikman <johan.wikman@mariadb.com>
Date: Mon, 19 Dec 2016 10:20:16 +0200
Subject: [PATCH] Replace gwbuf_clone_all with gwbuf_clone

gwbuf_clone cloned only the first buffer of a chain of buffers,
which never can be the desired outcome, while gwbuf_clone_all
cloned all buffers.

Now, gwbuf_clone behaves the way gwbuf_clone_all used to behave
and gwbuf_clone_all has been removed.
---
 include/maxscale/buffer.h                     |  1 -
 server/core/buffer.c                          | 43 ++++++++++++-------
 server/core/test/testbuffer.c                 |  2 +-
 server/modules/filter/tee/tee.c               |  4 +-
 .../readwritesplit/rwsplit_session_cmd.c      |  2 +-
 5 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/include/maxscale/buffer.h b/include/maxscale/buffer.h
index d303ce5d9..12c1500ca 100644
--- a/include/maxscale/buffer.h
+++ b/include/maxscale/buffer.h
@@ -200,7 +200,6 @@ extern size_t           gwbuf_copy_data(GWBUF *buffer, size_t offset, size_t byt
                                         uint8_t* dest);
 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);
 extern void             gwbuf_set_type(GWBUF *head, gwbuf_type_t type);
 extern int              gwbuf_add_property(GWBUF *buf, char *name, char *value);
 extern char             *gwbuf_get_property(GWBUF *buf, char *name);
diff --git a/server/core/buffer.c b/server/core/buffer.c
index ff753c1f7..abb484495 100644
--- a/server/core/buffer.c
+++ b/server/core/buffer.c
@@ -323,8 +323,8 @@ gwbuf_free_one(GWBUF *buf)
  * @param buf The buffer to use
  * @return A new GWBUF structure
  */
-GWBUF *
-gwbuf_clone(GWBUF *buf)
+static GWBUF *
+gwbuf_clone_one(GWBUF *buf)
 {
     GWBUF *rval;
 
@@ -350,31 +350,42 @@ gwbuf_clone(GWBUF *buf)
 }
 
 /**
- * Clone whole GWBUF list instead of single buffer.
+ * Clone a GWBUF. Note that if the GWBUF is actually a list of
+ * GWBUFs, then every GWBUF in the list will be cloned.
  *
- * @param buf   head of the list to be cloned till the tail of it
+ * @param buf  The GWBUF to be cloned.
  *
- * @return head of the cloned list or NULL if the list was empty.
+ * @return The cloned GWBUF, or NULL if @buf was NULL or if any part
+ *         of @buf could not be cloned.
  */
-GWBUF* gwbuf_clone_all(GWBUF* buf)
+GWBUF* gwbuf_clone(GWBUF* buf)
 {
-    GWBUF* rval;
-    GWBUF* clonebuf;
-
     if (buf == NULL)
     {
         return NULL;
     }
-    /** Store the head of the list to rval. */
-    clonebuf = gwbuf_clone(buf);
-    rval = clonebuf;
 
-    while (buf->next)
+    GWBUF *rval = gwbuf_clone_one(buf);
+
+    if (rval)
     {
-        buf = buf->next;
-        clonebuf->next = gwbuf_clone(buf);
-        clonebuf = clonebuf->next;
+        GWBUF* clonebuf = rval;
+
+        while (clonebuf && buf->next)
+        {
+            buf = buf->next;
+            clonebuf->next = gwbuf_clone(buf);
+            clonebuf = clonebuf->next;
+        }
+
+        if (!clonebuf && buf->next)
+        {
+            // A gwbuf_clone failed, we need to free everything cloned sofar.
+            gwbuf_free(rval);
+            rval = NULL;
+        }
     }
+
     return rval;
 }
 
diff --git a/server/core/test/testbuffer.c b/server/core/test/testbuffer.c
index ac645f1cd..92ed91d00 100644
--- a/server/core/test/testbuffer.c
+++ b/server/core/test/testbuffer.c
@@ -434,7 +434,7 @@ test1()
     ss_info_dassert(append == head, "gwbuf_append should return head");
     ss_info_dassert(append->next == tail, "After append tail should be in the next pointer of head");
     ss_info_dassert(append->tail == tail, "After append tail should be in the tail pointer of head");
-    GWBUF* all_clones = gwbuf_clone_all(head);
+    GWBUF* all_clones = gwbuf_clone(head);
     ss_info_dassert(all_clones && all_clones->next, "Cloning all should work");
     ss_info_dassert(GWBUF_LENGTH(all_clones) == headsize, "First buffer should be 10 bytes");
     ss_info_dassert(GWBUF_LENGTH(all_clones->next) == tailsize, "Second buffer should be 20 bytes");
diff --git a/server/modules/filter/tee/tee.c b/server/modules/filter/tee/tee.c
index d5f3e3fe4..5a11aa56b 100644
--- a/server/modules/filter/tee/tee.c
+++ b/server/modules/filter/tee/tee.c
@@ -916,7 +916,7 @@ GWBUF* clone_query(TEE_INSTANCE* my_instance, TEE_SESSION* my_session, GWBUF* bu
 
     if ((!my_instance->match && !my_instance->nomatch) || packet_is_required(buffer))
     {
-        clone = gwbuf_clone_all(buffer);
+        clone = gwbuf_clone(buffer);
     }
     else
     {
@@ -927,7 +927,7 @@ GWBUF* clone_query(TEE_INSTANCE* my_instance, TEE_SESSION* my_session, GWBUF* bu
             if ((my_instance->match && regexec(&my_instance->re, ptr, 0, NULL, 0) == 0) ||
                 (my_instance->nomatch && regexec(&my_instance->nore, ptr, 0, NULL, 0) != 0))
             {
-                clone = gwbuf_clone_all(buffer);
+                clone = gwbuf_clone(buffer);
             }
             MXS_FREE(ptr);
         }
diff --git a/server/modules/routing/readwritesplit/rwsplit_session_cmd.c b/server/modules/routing/readwritesplit/rwsplit_session_cmd.c
index c967aaf41..09676e2f7 100644
--- a/server/modules/routing/readwritesplit/rwsplit_session_cmd.c
+++ b/server/modules/routing/readwritesplit/rwsplit_session_cmd.c
@@ -321,7 +321,7 @@ GWBUF *sescmd_cursor_clone_querybuf(sescmd_cursor_t *scur)
     }
     ss_dassert(scur->scmd_cur_cmd != NULL);
 
-    buf = gwbuf_clone_all(scur->scmd_cur_cmd->my_sescmd_buf);
+    buf = gwbuf_clone(scur->scmd_cur_cmd->my_sescmd_buf);
 
     CHK_GWBUF(buf);
     return buf;