Re: [HACKERS] Possible gaps/garbage in the output of XLOG reader

[Michael Paquier]

On Thu, Apr 9, 2015 at 7:05 PM, Antonin Houska  wrote:
> While playing with xlogreader, I was lucky enough to see one of the many
> record validations to fail. After having some fun with gdb, I found out that
> in some cases the reader does not enforce enough data to be in state->readBuf
> before copying into state->readRecordBuf starts. This should not happen if the
> callback always reads XLOG_BLCKSZ bytes, but in fact only *reqLen* is the
> mandatory size of the chunk delivered.
>
> There are probably various ways to fix this problem. Attached is what I did in
> my environment. I hit the problem on 9.4.1, but the patch seems to apply to
> master too.

This looks similar to what is discussed here:
https://www.postgresql.org/message-id/flat/caboikdpspbymig6j01dkq6om2+bnkxhtpkoyqhm2a4oywgk...@mail.gmail.com
And there is a different patch which takes a lower-level approach, and
it seems to me cleaner approach in its way of calculating the record
offset when it goes through several XLOG pages.

Perhaps you could help review it? It is attached to the next commit fest.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Possible gaps/garbage in the output of XLOG reader

[Antonin Houska]

While playing with xlogreader, I was lucky enough to see one of the many
record validations to fail. After having some fun with gdb, I found out that
in some cases the reader does not enforce enough data to be in state-readBuf
before copying into state-readRecordBuf starts. This should not happen if the
callback always reads XLOG_BLCKSZ bytes, but in fact only *reqLen* is the
mandatory size of the chunk delivered.

There are probably various ways to fix this problem. Attached is what I did in
my environment. I hit the problem on 9.4.1, but the patch seems to apply to
master too.

-- 
Antonin Houska
Cybertec Schönig  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 474137a..e6ebd9d 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -313,7 +313,21 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
 		goto err;
 	}
 
-	len = XLOG_BLCKSZ - RecPtr % XLOG_BLCKSZ;
+	/* Bytes of the current record residing on the current page. */
+	len = Min(XLOG_BLCKSZ - targetRecOff, total_len);
+
+	/*
+	 * Nothing beyond the record header is guaranteed to be in state-readBuf
+	 * so far.
+	 */
+	if (readOff  targetRecOff + len)
+	{
+		readOff = ReadPageInternal(state, targetPagePtr, targetRecOff + len);
+
+		if (readOff  0)
+			goto err;
+	}
+
 	if (total_len  len)
 	{
 		/* Need to reassemble record */
@@ -322,9 +336,11 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
 		char	   *buffer;
 		uint32		gotlen;
 
+		Assert(readOff == targetRecOff + len);
+		Assert(readOff == XLOG_BLCKSZ);
+
 		/* Copy the first fragment of the record from the first page. */
-		memcpy(state-readRecordBuf,
-			   state-readBuf + RecPtr % XLOG_BLCKSZ, len);
+		memcpy(state-readRecordBuf, state-readBuf + targetRecOff, len);
 		buffer = state-readRecordBuf + len;
 		gotlen = len;
 
@@ -413,20 +429,16 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
 	}
 	else
 	{
-		/* Wait for the record data to become available */
-		readOff = ReadPageInternal(state, targetPagePtr,
- Min(targetRecOff + total_len, XLOG_BLCKSZ));
-		if (readOff  0)
-			goto err;
+		Assert(readOff = targetRecOff + len);
 
 		/* Record does not cross a page boundary */
 		if (!ValidXLogRecord(state, record, RecPtr))
 			goto err;
 
-		state-EndRecPtr = RecPtr + MAXALIGN(total_len);
+		state-EndRecPtr = RecPtr + MAXALIGN(len);
 
 		state-ReadRecPtr = RecPtr;
-		memcpy(state-readRecordBuf, record, total_len);
+		memcpy(state-readRecordBuf, record, len);
 	}
 
 	/*

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers