Fix two issues in TOAST decompression.

pglz_maximum_compressed_size() potentially underestimated the amount
of compressed data required to produce N bytes of decompressed data;
this is a fault in commit 11a078cf8.

Separately from that, pglz_decompress() failed to protect itself
against corrupt compressed data, particularly off == 0 in a match
tag.  Commit c60e520f6 turned such a situation into an infinite loop,
where before it'd just have resulted in garbage output.

The combination of these two bugs seems like it may explain bug #16694
from Tom Vijlbrief, though it's impossible to be quite sure without
direct inspection of the failing session.  (One needs to assume that
the pglz_maximum_compressed_size() bug caused us to fail to fetch the
second byte of a match tag, and what happened to be there instead was
a zero.  The reported infinite loop is hard to explain without off == 0,
though.)

Aside from fixing the bugs, rewrite associated comments for more
clarity.

Back-patch to v13 where both these commits landed.

Discussion: https://postgr.es/m/16694-f107871e499ec114@postgresql.org

diff --git a/src/common/pg_lzcompress.c b/src/common/pg_lzcompress.c
index d24d4803a9839fff48c36d46aadc5854fd5fad66..79747767ce08a0144b85d3496d8def6b74071825 100644 (file)
--- a/src/common/pg_lzcompress.c
+++ b/src/common/pg_lzcompress.c
@@ -710,7 +710,6 @@ pglz_decompress(const char *source, int32 slen, char *dest,
 
                for (ctrlc = 0; ctrlc < 8 && sp < srcend && dp < destend; ctrlc++)
                {
-
                        if (ctrl & 1)
                        {
                                /*
@@ -732,41 +731,62 @@ pglz_decompress(const char *source, int32 slen, char *dest,
                                        len += *sp++;
 
                                /*
-                                * Now we copy the bytes specified by the tag from OUTPUT to
-                                * OUTPUT (copy len bytes from dp - off to dp). The copied
-                                * areas could overlap, to preven possible uncertainty, we
-                                * copy only non-overlapping regions.
+                                * Check for corrupt data: if we fell off the end of the
+                                * source, or if we obtained off = 0, we have problems.  (We
+                                * must check this, else we risk an infinite loop below in the
+                                * face of corrupt data.)
+                                */
+                               if (sp > srcend || off == 0)
+                                       break;
+
+                               /*
+                                * Don't emit more data than requested.
                                 */
                                len = Min(len, destend - dp);
+
+                               /*
+                                * Now we copy the bytes specified by the tag from OUTPUT to
+                                * OUTPUT (copy len bytes from dp - off to dp).  The copied
+                                * areas could overlap, so to avoid undefined behavior in
+                                * memcpy(), be careful to copy only non-overlapping regions.
+                                *
+                                * Note that we cannot use memmove() instead, since while its
+                                * behavior is well-defined, it's also not what we want.
+                                */
                                while (off < len)
                                {
-                                       /*---------
-                                        * When offset is smaller than length - source and
-                                        * destination regions overlap. memmove() is resolving
-                                        * this overlap in an incompatible way with pglz. Thus we
-                                        * resort to memcpy()-ing non-overlapping regions.
-                                        *
-                                        * Consider input: 112341234123412341234
-                                        * At byte 5       here ^ we have match with length 16 and
-                                        * offset 4.       11234M(len=16, off=4)
-                                        * We are decoding first period of match and rewrite match
-                                        *                 112341234M(len=12, off=8)
-                                        *
-                                        * The same match is now at position 9, it points to the
-                                        * same start byte of output, but from another position:
-                                        * the offset is doubled.
-                                        *
-                                        * We iterate through this offset growth until we can
-                                        * proceed to usual memcpy(). If we would try to decode
-                                        * the match at byte 5 (len=16, off=4) by memmove() we
-                                        * would issue memmove(5, 1, 16) which would produce
-                                        * 112341234XXXXXXXXXXXX, where series of X is 12
-                                        * undefined bytes, that were at bytes [5:17].
-                                        *---------
+                                       /*
+                                        * We can safely copy "off" bytes since that clearly
+                                        * results in non-overlapping source and destination.
                                         */
                                        memcpy(dp, dp - off, off);
                                        len -= off;
                                        dp += off;
+
+                                       /*----------
+                                        * This bit is less obvious: we can double "off" after
+                                        * each such step.  Consider this raw input:
+                                        *              112341234123412341234
+                                        * This will be encoded as 5 literal bytes "11234" and
+                                        * then a match tag with length 16 and offset 4.  After
+                                        * memcpy'ing the first 4 bytes, we will have emitted
+                                        *              112341234
+                                        * so we can double "off" to 8, then after the next step
+                                        * we have emitted
+                                        *              11234123412341234
+                                        * Then we can double "off" again, after which it is more
+                                        * than the remaining "len" so we fall out of this loop
+                                        * and finish with a non-overlapping copy of the
+                                        * remainder.  In general, a match tag with off < len
+                                        * implies that the decoded data has a repeat length of
+                                        * "off".  We can handle 1, 2, 4, etc repetitions of the
+                                        * repeated string per memcpy until we get to a situation
+                                        * where the final copy step is non-overlapping.
+                                        *
+                                        * (Another way to understand this is that we are keeping
+                                        * the copy source point dp - off the same throughout.)
+                                        *----------
+                                        */
                                        off += off;
                                }
                                memcpy(dp, dp - off, len);
@@ -820,21 +840,32 @@ pglz_decompress(const char *source, int32 slen, char *dest,
 int32
 pglz_maximum_compressed_size(int32 rawsize, int32 total_compressed_size)
 {
-       int32           compressed_size;
+       int64           compressed_size;
 
        /*
-        * pglz uses one control bit per byte, so we need (rawsize * 9) bits. We
-        * care about bytes though, so we add 7 to make sure we include the last
-        * incomplete byte (integer division rounds down).
+        * pglz uses one control bit per byte, so if the entire desired prefix is
+        * represented as literal bytes, we'll need (rawsize * 9) bits.  We care
+        * about bytes though, so be sure to round up not down.
         *
-        * XXX Use int64 to prevent overflow during calculation.
+        * Use int64 here to prevent overflow during calculation.
+        */
+       compressed_size = ((int64) rawsize * 9 + 7) / 8;
+
+       /*
+        * The above fails to account for a corner case: we could have compressed
+        * data that starts with N-1 or N-2 literal bytes and then has a match tag
+        * of 2 or 3 bytes.  It's therefore possible that we need to fetch 1 or 2
+        * more bytes in order to have the whole match tag.  (Match tags earlier
+        * in the compressed data don't cause a problem, since they should
+        * represent more decompressed bytes than they occupy themselves.)
         */
-       compressed_size = (int32) ((int64) rawsize * 9 + 7) / 8;
+       compressed_size += 2;
 
        /*
         * Maximum compressed size can't be larger than total compressed size.
+        * (This also ensures that our result fits in int32.)
         */
        compressed_size = Min(compressed_size, total_compressed_size);
 
-       return compressed_size;
+       return (int32) compressed_size;
 }