BUG/MINOR: slz: use the exact switch cost for the last literals of a block

The decision to send the pending literals as a stored block rather than in
fixed huffman mode is taken when the 9-bit literals wasted more than the
52 bits it costs to leave the fixed huffman encoding and to come back to
it. But for the last literals of a block, nothing comes after the stored
block, so there is no need to pay for the block type of a next block nor
for the EOB, while the huffman variant still has to send an EOB. The
switch is thus 10 bits cheaper, and 10 more when the stream is still in
EOB state, since then the block type is needed in both cases and no EOB
has to be terminated.

Using 52 there made the encoder prefer huffman for data that was cheaper
to store, and the output could exceed the documented maximum. The smallest
case found by fuzzing is a 47-byte input entirely made of bytes >= 144
which produced 55 bytes (3 bits of block type + 47*9 bits + 7 bits of EOB)
where the stored block only needs 52, for a documented maximum of 54.

With these correct costs, we no longer see outputs exceed the documented
maximum, wether it's with small inputs (tested with ~3 million random
small inputs as small as 47 bytes), or usual files found in tests/ and
bash, gcc, libc, and silesia. No performance change was observed either.

Note that a stream can still exceed the documented maximum by a few bytes
(17 bytes were observed on a 390000-byte crafted input) because each
reference emitted between two stored blocks forces them out and adds a
5-byte block header that the accounting attributes to the reference. This
is for a future fix.

This should be backported to 1.2.

This is libslz upstream commit 97757536178f24aeb2cb41278706a88c1242f414.

Note: the impact in haproxy is practically inexistent since we have a 1kB
      reserve and build by default with the tag at the end of pools, even
      if two extra bytes were to be emitted, it would have no effect.
      Better backport it to avoid triggering ASAN though.

diff --git a/src/slz.c b/src/slz.c
index e21a609b45fa9613e758efee5476e2c2ddba537e..20ba2303664325e53a2b1e61966c6308cb15c19f 100644 (file)
--- a/src/slz.c
+++ b/src/slz.c
@@ -507,6 +507,22 @@ static void reset_refs(union ref *refs, long count)
        } while (refs < end);
 }
 
+/* Number of bits wasted by the 9-bit literals above which it becomes
+ * preferable to send the pending literals as a stored block. It corresponds to
+ * the cost of leaving the fixed huffman encoding and coming back to it: EOB
+ * (7 bits) + block type (3 bits) + alignment (up to 7 bits) + LEN and NLEN
+ * (32 bits) + block type of the next block (3 bits), that is 52 bits. Two
+ * cheaper variants are needed for the last literals of a block: nothing
+ * follows the stored block while the huffman encoding still has to send an EOB
+ * (10 bits less), and if the stream is still in EOB state, no EOB has to be
+ * sent and the block type is needed in both cases (10 more bits less). Using a
+ * larger value than these would leave the output larger than the same data
+ * sent as stored blocks, in violation of the promise made above.
+ */
+#define SLZ_SWITCH_COST      52
+#define SLZ_LAST_COST        42
+#define SLZ_LAST_COST_EOB    32
+
 /* Compresses <ilen> bytes from <in> into <out> according to RFC1951. The
  * output result may be up to 5 bytes larger than the input for each 65535
  * nput bytes, to which 2 extra bytes may be added to send the last chunk due
@@ -691,7 +707,8 @@ long slz_rfc1951_encode(struct slz_stream *strm, unsigned char *out, const unsig
                 * is what guarantees that once switched to FIXED we can stay
                 * in it for as long as needed.
                 */
-               if (strm->state == SLZ_ST_EOB && (dist & 0x1f) + (code >> 16) + 52 > 8 * mlen)
+               if (strm->state == SLZ_ST_EOB &&
+                   (dist & 0x1f) + (code >> 16) + SLZ_SWITCH_COST > 8 * mlen)
                        goto send_as_lit;
 
                /* first, copy pending literals */
@@ -703,7 +720,7 @@ long slz_rfc1951_encode(struct slz_stream *strm, unsigned char *out, const unsig
                         * block. Only use plain literals if there are more than 52 bits
                         * to save then.
                         */
-                       if (bit9 >= 52)
+                       if (bit9 >= SLZ_SWITCH_COST)
                                copy_lit(strm, in + pos - plit, plit, 1);
                        else
                                copy_lit_huff(strm, in + pos - plit, plit, 1);
@@ -749,9 +766,19 @@ long slz_rfc1951_encode(struct slz_stream *strm, unsigned char *out, const unsig
        }
 
  final_lit_dump:
-       /* now copy remaining literals or mark the end */
+       /* Now copy remaining literals or mark the end. The cost of switching to
+        * a stored block depends on the current state and on the presence of
+        * data after these literals, see the SLZ_*_COST definitions above.
+        */
        if (plit) {
-               if (bit9 >= 52)
+               uint32_t cost;
+
+               if (more)
+                       cost = SLZ_SWITCH_COST;
+               else
+                       cost = (strm->state == SLZ_ST_EOB) ? SLZ_LAST_COST_EOB : SLZ_LAST_COST;
+
+               if (bit9 >= cost)
                        copy_lit(strm, in + pos - plit, plit, more);
                else
                        copy_lit_huff(strm, in + pos - plit, plit, more);