Merge lp:~gl-az/percona-xtrabackup/bug1273196-2.1 into lp:percona-xtrabackup/2.1

[George Ormond Lorch III]

bug 1273196 - Streaming issue with xbcrypt

xbcrypt read failures are due to partial reads happening on a stdin file because reads are not using either MY_FULL_IO nor looping and rereading on a partial (> 0) read.

To fix, simple re-read loops are added for each read point to progressively continue to read into the buffer as long as something was read.

jenkins http://jenkins.percona.com/view/PXB%202.1/job/percona-xtrabackup-2.1-param/542/

[Sergei Glushchenko (sergei.glushchenko)]

Hi George,

Looks like crypt->read use my_read to actually read data. Since MY_FULL_IO is not passed to my_read, it can return MY_FILE_ERROR in case of error (which is a big number). I think F_READ doesn't handle this case properly.

review: Needs Fixing (g2)

[George Ormond Lorch III (gl-az)]

Good catch Sergei, thanks! A simple fix and new jenkins run: http://jenkins.percona.com/job/percona-xtrabackup-2.1-param/547/

[Sergei Glushchenko (sergei.glushchenko)]

Approve

review: Approve (g2)

[Alexey Kopytov (akopytov)]

The fix looks correct, but I don't understand the purpose of another wrapper. Why not embed all F_READ() logic into my_xb_crypt_read_callback()?

[George Ormond Lorch III (gl-az)]

> The fix looks correct, but I don't understand the purpose of another wrapper.
> Why not embed all F_READ() logic into my_xb_crypt_read_callback()?

That is a very good question. Why don't I? I'll get that changed up, jenkinsed and all real quick here.

[Alexey Kopytov (akopytov)]

George,

I took care of that in my fix for bug #1273207, because I either had to duplicate your fix, or generalize it.

=== modified file 'src/xbcrypt_read.c'
--- src/xbcrypt_read.c	2013-10-17 01:00:57 +0000
+++ src/xbcrypt_read.c	2014-02-25 17:15:13 +0000
@@ -49,6 +49,27 @@
 	return crypt;
 }
 
+static
+size_t
+F_READ(xb_rcrypt_t *crypt, void *buf, size_t len)
+{
+	uchar		*ptr;
+	size_t		bytesread;
+	size_t		bytestoread;
+
+	ptr = buf;
+	bytestoread = len;
+	while (bytestoread > 0) {
+		bytesread = crypt->read(crypt->userdata, ptr, bytestoread, MYF(MY_WME));
+		if (bytesread == 0 || bytesread > bytestoread) {
+			break;
+		}
+		ptr += bytesread;
+		bytestoread -= bytesread;
+	}
+	return len - bytestoread;
+}
+
 xb_rcrypt_result_t
 xb_crypt_read_chunk(xb_rcrypt_t *crypt, void **buf, size_t *olen, size_t *elen,
 		    void **iv, size_t *ivlen)
@@ -58,21 +79,19 @@
 	uchar		*ptr;
 	ulonglong	tmp;
 	ulong		checksum, checksum_exp, version;
-	ssize_t		bytesread;
+	size_t		bytesread;
 	xb_rcrypt_result_t result = XB_CRYPT_READ_CHUNK;
 
-	if ((bytesread = crypt->read(crypt->userdata, tmpbuf, sizeof(tmpbuf), MYF(MY_WME)))
-	    != sizeof(tmpbuf)) {
-		if (bytesread == 0) {
-			result = XB_CRYPT_READ_EOF;
-			goto err;
-		} else {
-			msg("%s:%s: unable to read chunk header data at "
-			    "offset 0x%llx.\n",
-			    my_progname, __FUNCTION__, crypt->offset);
-			result =  XB_CRYPT_READ_ERROR;
-			goto err;
-		}
+	bytesread = F_READ(crypt, tmpbuf, sizeof(tmpbuf));
+	if (bytesread == 0) {
+		result = XB_CRYPT_READ_EOF;
+		goto err;
+	} else if (bytesread < sizeof(tmpbuf)) {
+		msg("%s:%s: unable to read chunk header data at "
+		    "offset 0x%llx.\n",
+		    my_progname, __FUNCTION__, crypt->offset);
+		result =  XB_CRYPT_READ_ERROR;
+		goto err;
 	}
 
 	ptr = tmpbuf;
@@ -126,18 +145,16 @@
 		*ivlen = 0;
 		*iv = 0;
 	} else {
-		if ((bytesread = crypt->read(crypt->userdata, tmpbuf, 8,
-					     MYF(MY_WME))) != 8) {
-			if (bytesread == 0) {
-				result = XB_CRYPT_READ_EOF;
-				goto err;
-			} else {
-				msg("%s:%s: unable to read chunk iv size at "
-				    "offset 0x%llx.\n",
-				    my_progname, __FUNCTION__, crypt->offset);
-				result =  XB_CRYPT_READ_ERROR;
-				goto err;
-			}
+		bytesread = F_READ(crypt, tmpbuf, 8);
+		if (bytesread == 0) {
+			result = XB_CRYPT_READ_EOF;
+			goto err;
+		} else if (bytesread < 8) {
+			msg("%s:%s: unable to read chunk iv size at "
+			    "offset 0x%llx.\n",
+			    my_progname, __FUNCTION__, crypt->offset);
+			result =  XB_CRYPT_READ_ERROR;
+			goto err;
 		}
 
 		tmp = uint8korr(tmpbuf);
@@ -176,8 +193,11 @@
 	}
 
 	if (*ivlen > 0) {
-		if (crypt->read(crypt->userdata, crypt->ivbuffer, *ivlen, MYF(MY_WME|MY_FULL_IO))
-		    != (ssize_t)*ivlen) {
+		bytesread = F_READ(crypt, crypt->ivbuffer, *ivlen);
+		if (bytesread == 0) {
+			result = XB_CRYPT_READ_EOF;
+			goto err;
+		} else if (bytesread < *ivlen) {
 			msg("%s:%s: failed to read %lld bytes for chunk iv "
 			    "at offset 0x%llx.\n", my_progname, __FUNCTION__,
 			    (ulonglong)*ivlen, crypt->offset);
@@ -212,8 +232,11 @@
 	}
 
 	if (*elen > 0) {
-		if (crypt->read(crypt->userdata, crypt->buffer, *elen, MYF(MY_WME|MY_FULL_IO))
-		    != (ssize_t)*elen) {
+		bytesread = F_READ(crypt, crypt->buffer, *elen);
+		if (bytesread == 0) {
+			result = XB_CRYPT_READ_EOF;
+			goto err;
+		} else if (bytesread < *elen) {
 			msg("%s:%s: failed to read %lld bytes for chunk payload "
 			    "at offset 0x%llx.\n", my_progname, __FUNCTION__,
 			    (ulonglong)*elen, crypt->offset);