Horton

Re: BUG #16476: pgp_sym_encrypt_bytea with compress-level=6 : Wrong key or corrupt data

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Mark Wong <mark@2ndquadrant.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, jeff.janes@gmail.com, frank.gagnepain@intm.fr, pgsql-bugs@lists.postgresql.org
Date: 2020-07-23T19:38:43Z
Lists: pgsql-bugs 
I dug into this problem with access kindly provided by Mark Wong, and
verified that indeed the zlib on that machine acts a bit differently
from stock zlib.  The problem we're facing turns out to be entirely
unrelated to the patch at hand; it's the *compression* side that is
misbehaving.  After some digging in the code and reading the zlib.h
API spec carefully, the answer is that compress_process() completely
mishandles the situation where deflate() stops short of consuming all
the input that's supplied.  It resets the next_in pointer so that
old data is reprocessed, rather than allowing the remaining unprocessed
data to be processed.  We need to do this:

--- a/contrib/pgcrypto/pgp-compress.c
+++ b/contrib/pgcrypto/pgp-compress.c
@@ -114,10 +114,10 @@ compress_process(PushFilter *next, void *priv, const uint8 *data, int len)
 	/*
 	 * process data
 	 */
-	while (len > 0)
+	st->stream.next_in = unconstify(uint8 *, data);
+	st->stream.avail_in = len;
+	while (st->stream.avail_in > 0)
 	{
-		st->stream.next_in = unconstify(uint8 *, data);
-		st->stream.avail_in = len;
 		st->stream.next_out = st->buf;
 		st->stream.avail_out = st->buf_len;
 		res = deflate(&st->stream, 0);
@@ -131,7 +131,6 @@ compress_process(PushFilter *next, void *priv, const uint8 *data, int len)
 			if (res < 0)
 				return res;
 		}
-		len = st->stream.avail_in;
 	}
 
 	return 0;

I suppose this has been broken since day one; it's a bit astonishing (and
disheartening) that nobody's reported a problem here before.

Anyway, with that corrected, the SLES zlib still produces different
output from stock zlib, and indeed seemingly is worse: the result
is noticeably larger than what stock zlib produces.  It does decompress
back to the same thing, though, so this is a performance problem not
a data corruption issue.  That does mean that the proposed test case
fails to exercise our empty-ending-block scenario with this version
of zlib.  I don't think we really care about that, though.

I will go apply this fix, and then you can put back the fix for
the originally-reported problem.  I still like Horiguchi-san's
fix better than what was committed, though.

			regards, tom lane

Commits

  1. Fix corner case with 16kB-long decompression in pgcrypto, take 2

  2. Fix ancient violation of zlib's API spec.

  3. Revert "Fix corner case with PGP decompression in pgcrypto"

  4. Fix corner case with PGP decompression in pgcrypto