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: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, jeff.janes@gmail.com, frank.gagnepain@intm.fr, pgsql-bugs@lists.postgresql.org, Mark Wong <mark@2ndquadrant.com>
Date: 2020-07-22T22:38:50Z
Lists: pgsql-bugs 
I've spent quite some time digging around in pgcrypto, and I can't
find anything that looks clearly machine-dependent in the code paths
involved here.  The only suspicious thing I've found is that (at least
for most of the PullFilters) pullf_read will not initialize the
output data pointer if no bytes are returned.  So on the last call
where we expect to get an EOF result from the subsidiary filter,
this:

		uint8	   *tmp;

		res = pullf_read(src, 8192, &tmp);
		if (res < 0)
			return res;
		dec->stream.next_in = tmp;
		dec->stream.avail_in = res;

is setting next_in to garbage.  However, if we've already set
eof to 1, which we have, then we won't call inflate() again
so the garbage pointer should not matter.  (Besides which,
zlib really shouldn't dereference that pointer if avail_in is 0.)
I'm still baffled as to why only the SLES s390x animals are failing,
but it's beginning to seem like it might be due to them using a
different zlib version.

Having said that, though, I do not like the committed patch one bit.
It's got two big problems:

1. Once EOF has been detected, we'll still call the subsidiary filter
again on each subsequent call to decompress_read, if it takes several
calls to empty out the final load of decompressed data.  This is only
safe if all the pgcrypto filter types are okay with being called again
after they report EOF.  I'm not sure that is true --- mdc_read looks
like a counterexample --- and even if it is true, it seems inefficient.

2. There is no check to make sure that we got an EOF indication from
the subsidiary filter.  If it returned some bytes, those will just be
absorbed into the decompression input buffer, but we'll never try to
decompress them; they're just lost.  This is the inverse of the bug
allegedly being fixed here: instead of reading too few bytes and not
caring, we read too many and don't care.  Either way we lose sync
with the incoming data stream, causing problems at higher filter
levels.

Horiguchi-san's v3 patch at
<20200612.145412.475791851624925277.horikyota.ntt@gmail.com>
doesn't have either of these problems, so it seems much superior
to me than what you actually did.  I don't have a lot of hope that
changing to that one would fix the buildfarm problem --- but maybe it
would, if the machine-dependent behavior is somehow hiding in the
repeat-call-after-EOF code path.

			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