BUG/MINOR: slz: do not append a block to an already finished stream

slz_rfc1951_flush() terminates the pending block then emits an empty
stored block to byte-align the output. But encode() called with <more>
cleared may leave the stream in SLZ_ST_LAST, that is, inside a block whose
BFINAL bit has already been sent. Flushing there terminated that block,
which completes the deflate stream, and then emitted one more block with
BFINAL set again. Those 5 bytes sit past the end of the stream, so the
gzip or zlib trailer that finish() appends right after is no longer where
the peer expects it: it reads the empty block as the checksum.

The data itself decodes correctly, only the check fails, which makes it
particularly difficult to diagnose. Raw deflate is not affected as it has
no trailer to shift, and a stream that ends in EOB state is not affected
either since its queue is empty and flush() returns early. Fuzzing random
sequences of encode/flush/finish showed ~2% corrupt streams for gzip and
zlib.

When the terminated block was the final one there is nothing left to align
to, and nothing may be appended, so let's just flush the pending bits and
return. This also makes the flush 4 bytes shorter in that case, and the
BFINAL bit of the empty block is now always zero, which is what the state
guarantees at that point.

This must be backported to 1.2.

This is libslz upstream commit 12e726390c96a21bd910d02a2fc594bcd743c131.

Note: no practical impact in haproxy since we never send streams back-
      to-back, but some deflate stream decoders might occasionally spot
      an error.

diff --git a/src/slz.c b/src/slz.c
index 1c406c662025c9256c5eeff79a10c21a291a5c94..f50946ff100e9099c77fc7cc7f8974860978aa6e 100644 (file)
--- a/src/slz.c
+++ b/src/slz.c
@@ -853,9 +853,12 @@ int slz_rfc1951_init(struct slz_stream *strm, int level)
  * queue (up to 31 bits when using the 64-bit queue), plus a possible EOB (7
  * bits), plus (BFINAL,BTYPE) (3 bits), which once rounded up to the next byte
  * make 6 bytes, plus 4 bytes for LEN+NLEN, or a total of 10 bytes in the worst
- * case. The number of bytes emitted is returned. It is guaranteed that the
- * queue is empty on return. This may cause some overhead by adding needless
- * 5-byte blocks if called to often.
+ * case. If the stream was already completed by a previous call to encode()
+ * with <more> cleared, the last block is simply terminated and the output
+ * aligned, since nothing may be appended to a finished stream. The number of
+ * bytes emitted is returned. It is guaranteed that the queue is empty on
+ * return. This may cause some overhead by adding needless 5-byte blocks if
+ * called to often.
  */
 int slz_rfc1951_flush(struct slz_stream *strm, unsigned char *buf)
 {
@@ -871,15 +874,26 @@ int slz_rfc1951_flush(struct slz_stream *strm, unsigned char *buf)
                send_eob(strm);
        }
 
-       /* send BFINAL according to state, and BTYPE=00 (lit) */
-       enqueue8(strm, (strm->state == SLZ_ST_DONE) ? 1 : 0, 3);
+       if (strm->state == SLZ_ST_DONE) {
+               /* The block we've just terminated was already marked final, so
+                * the deflate stream is complete and nothing may be appended to
+                * it: an extra block here would be located past the end of the
+                * stream and would shift the gzip or zlib trailer that the
+                * caller is about to emit. Aligning the output is all we have
+                * to do, and it's all that's left to do for finish() as well.
+                */
+               flush_bits(strm);
+               return strm->outbuf - buf;
+       }
+
+       /* send BFINAL=0 and BTYPE=00 (lit) */
+       enqueue8(strm, 0, 3);
        flush_bits(strm);             // emit pending bits
        copy_32b(strm, 0xFFFF0000U);  // len=0, nlen=~0
 
-       /* Now the queue is empty, EOB was sent, BFINAL might have been sent if
-        * we completed the last block, and a zero-byte block was sent to byte-
-        * align the output. The last state reflects all this. Let's just
-        * return the number of bytes added to the output buffer.
+       /* Now the queue is empty, EOB was sent, and a zero-byte block was sent
+        * to byte-align the output. The last state reflects all this. Let's
+        * just return the number of bytes added to the output buffer.
         */
        return strm->outbuf - buf;
 }