compound fops: Fix file corruption issue

1. Address of a local variable @args is copied into state->req
in server3_3_compound (). But even after the function has gone out of
scope, in server_compound_resume () this pointer is accessed and
dereferenced. This patch fixes that.

2. Compound fops, by virtue of NOT having a vector sizer (like the one
writev has), ends up having both the header and the data (in case one of
its member fops is WRITEV) in the same hdr_iobuf. This buffer was not
being preserved through the lifetime of the compound fop, causing it to
be overwritten by a parallel write fop, even when the writev associated
with the currently executing compound fop is yet to hit the desk, thereby
corrupting the file's data. This is fixed by associating the hdr_iobuf with
the iobref so its memory remains valid through the lifetime of the fop.

3. Also fixed a use-after-free bug in protocol/client in compound fops cbk,
missed by Linux but caught by NetBSD.

Finally, big thanks to Pranith Kumar K and Raghavendra Gowdappa for their
help in debugging this file corruption issue.

Change-Id: I6d5c04f400ecb687c9403a17a12683a96c2bf122
BUG: 1378778
Signed-off-by: Krutika Dhananjay <kdhananj@redhat.com>
Reviewed-on: http://review.gluster.org/15654
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
Reviewed-by: Raghavendra G <rgowdapp@redhat.com>
Smoke: Gluster Build System <jenkins@build.gluster.org>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>

8 files changed, 46 insertions, 22 deletions

diff --git a/rpc/rpc-lib/src/rpc-transport.c b/rpc/rpc-lib/src/rpc-transport.c
index 005b68c5cbc..6ee5e15ede4 100644
--- a/rpc/rpc-lib/src/rpc-transport.c
+++ b/rpc/rpc-lib/src/rpc-transport.c
@@ -123,10 +123,6 @@ rpc_transport_pollin_destroy (rpc_transport_pollin_t *pollin)
                 iobref_unref (pollin->iobref);
         }
 
-        if (pollin->hdr_iobuf) {
-                iobuf_unref (pollin->hdr_iobuf);
-        }
-
         if (pollin->private) {
                 /* */
                 GF_FREE (pollin->private);
@@ -158,7 +154,7 @@ rpc_transport_pollin_alloc (rpc_transport_t *this, struct iovec *vector,
         msg->iobref = iobref_ref (iobref);
         msg->private = private;
         if (hdr_iobuf)
-                msg->hdr_iobuf = iobuf_ref (hdr_iobuf);
+                iobref_add (iobref, hdr_iobuf);
 
 out:
         return msg;
diff --git a/rpc/rpc-lib/src/rpc-transport.h b/rpc/rpc-lib/src/rpc-transport.h
index f0add065065..4e7a8c46fae 100644
--- a/rpc/rpc-lib/src/rpc-transport.h
+++ b/rpc/rpc-lib/src/rpc-transport.h
@@ -163,7 +163,6 @@ struct rpc_transport_pollin {
         char vectored;
         void *private;
         struct iobref *iobref;
-        struct iobuf  *hdr_iobuf;
         char is_reply;
 };
 typedef struct rpc_transport_pollin rpc_transport_pollin_t;
diff --git a/rpc/rpc-lib/src/rpcsvc.c b/rpc/rpc-lib/src/rpcsvc.c
index f07e745a4b3..c792909cb87 100644
--- a/rpc/rpc-lib/src/rpcsvc.c
+++ b/rpc/rpc-lib/src/rpcsvc.c
@@ -373,9 +373,6 @@ rpcsvc_request_destroy (rpcsvc_request_t *req)
                 iobref_unref (req->iobref);
         }
 
-        if (req->hdr_iobuf)
-                iobuf_unref (req->hdr_iobuf);
-
         /* This marks the "end" of an RPC request. Reply is
            completely written to the socket and is on the way
            to the client. It is time to decrement the
@@ -690,9 +687,6 @@ rpcsvc_handle_rpc_call (rpcsvc_t *svc, rpc_transport_t *trans,
                 }
 
                 if (req->synctask) {
-                        if (msg->hdr_iobuf)
-                                req->hdr_iobuf = iobuf_ref (msg->hdr_iobuf);
-
                         ret = synctask_new (THIS->ctx->env,
                                             (synctask_fn_t) actor_fn,
                                             rpcsvc_check_and_reply_error, NULL,
diff --git a/rpc/rpc-lib/src/rpcsvc.h b/rpc/rpc-lib/src/rpcsvc.h
index 02e467e68a7..63a6dad8c2f 100644
--- a/rpc/rpc-lib/src/rpcsvc.h
+++ b/rpc/rpc-lib/src/rpcsvc.h
@@ -244,9 +244,6 @@ struct rpcsvc_request {
         /* Container for transport to store request-specific item */
         void                    *trans_private;
 
-        /* we need to ref the 'iobuf' in case of 'synctasking' it */
-        struct iobuf            *hdr_iobuf;
-
         /* pointer to cached reply for use in DRC */
         drc_cached_op_t         *reply;
 };
diff --git a/tests/basic/afr/compounded-write-txns.t b/tests/basic/afr/compounded-write-txns.t
new file mode 100644
index 00000000000..7cecd87b01b
--- /dev/null
+++ b/tests/basic/afr/compounded-write-txns.t
@@ -0,0 +1,37 @@
+#!/bin/bash
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+cleanup
+
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume create $V0 replica 3 $H0:$B0/${V0}{0,1,2}
+TEST $CLI volume set $V0 write-behind off
+TEST $CLI volume set $V0 client-io-threads off
+TEST $CLI volume start $V0
+TEST $GFS --volfile-id=$V0 --volfile-server=$H0 $M0
+
+# Create and generate data into a src file
+
+TEST `printf %1024s |tr " " "1" > /tmp/source`
+TEST `printf %1024s |tr " " "2" >> /tmp/source`
+
+TEST dd if=/tmp/source of=$M0/file bs=1024 count=2 2>/dev/null
+md5sum_file=$(md5sum $M0/file | awk '{print $1}')
+
+TEST $CLI volume set $V0 cluster.use-compound-fops on
+
+TEST dd if=$M0/file of=$M0/file-copy bs=1024 count=2 2>/dev/null
+
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+TEST $GFS --volfile-id=$V0 --volfile-server=$H0 $M0
+
+EXPECT "$md5sum_file" echo `md5sum $M0/file-copy | awk '{print $1}'`
+
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+TEST $CLI volume stop $V0
+TEST $CLI volume delete $V0
+
+TEST rm -f /tmp/source
+cleanup
diff --git a/xlators/protocol/client/src/client-rpc-fops.c b/xlators/protocol/client/src/client-rpc-fops.c
index b47d2268391..2d1b9540df4 100644
--- a/xlators/protocol/client/src/client-rpc-fops.c
+++ b/xlators/protocol/client/src/client-rpc-fops.c
@@ -3153,7 +3153,8 @@ client3_3_compound_cbk (struct rpc_req *req, struct iovec *iov, int count,
         xlator_t                *this            = NULL;
         dict_t                  *xdata           = NULL;
         clnt_local_t            *local           = NULL;
-        int                     i,length         = 0;
+        int                     i                = 0;
+        int                     length           = 0;
         int                     ret              = -1;
 
         this = THIS;
@@ -3176,12 +3177,12 @@ client3_3_compound_cbk (struct rpc_req *req, struct iovec *iov, int count,
                 goto out;
         }
 
+        length =  local->length;
+
         GF_PROTOCOL_DICT_UNSERIALIZE (this, xdata, (rsp.xdata.xdata_val),
                                       (rsp.xdata.xdata_len), rsp.op_ret,
                                        rsp.op_errno, out);
 
-        length =  local->length;
-
         args_cbk = compound_args_cbk_alloc (length, xdata);
         if (!args_cbk) {
                 rsp.op_ret   = -1;
@@ -3214,7 +3215,7 @@ out:
 
         free (rsp.xdata.xdata_val);
 
-        client_compound_rsp_cleanup (&rsp, local->length);
+        client_compound_rsp_cleanup (&rsp, length);
 
         if (xdata)
                 dict_unref (xdata);
diff --git a/xlators/protocol/server/src/server-rpc-fops.c b/xlators/protocol/server/src/server-rpc-fops.c
index 43061b03c56..25f575d3c6a 100644
--- a/xlators/protocol/server/src/server-rpc-fops.c
+++ b/xlators/protocol/server/src/server-rpc-fops.c
@@ -3324,7 +3324,7 @@ server_compound_resume (call_frame_t *frame, xlator_t *bound_xl)
                 goto err;
         }
 
-        req = state->req;
+        req = &state->req;
 
         length = req->compound_req_array.compound_req_array_len;
         state->args = compound_fop_alloc (length, req->compound_fop_enum,
@@ -6725,7 +6725,7 @@ server3_3_compound (rpcsvc_request_t *req)
                 goto out;
         }
 
-        state->req           = &args;
+        state->req           = args;
         state->iobref        = iobref_ref (req->iobref);
 
         if (len < req->msg[0].iov_len) {
diff --git a/xlators/protocol/server/src/server.h b/xlators/protocol/server/src/server.h
index c87dbe67b12..0b37eb1414a 100644
--- a/xlators/protocol/server/src/server.h
+++ b/xlators/protocol/server/src/server.h
@@ -191,7 +191,7 @@ struct _server_state {
         struct gf_lease   lease;
         lock_migration_info_t locklist;
         /* required for compound fops */
-        gfs3_compound_req *req;
+        gfs3_compound_req  req;
         /* last length till which iovec for compound
          * writes was processed */
         int               write_length;