BUG/MINOR: slz: do not read past the end of the input around the match loop

slz_rfc1951_encode() pre-loads the first 3 bytes of the input into <word>
before entering the main loop on architectures which do not define
UNALIGNED_FASTER (e.g. i386, or big endian ones). This was done
unconditionally, thus inputs shorter than 3 bytes (including empty ones)
caused up to 3 bytes to be read past the end of the input buffer, which
may segfault if the buffer ends on the last page of a mapping. This is
easily reproduced on i386 by placing a zero-length input right before an
unmapped page.

The pre-loaded word is only ever used if the main loop is entered, which
requires at least 4 remaining bytes, so let's simply condition the load
on this.

The exact same case exists at the end of the loop where we can go beyond
end-3 and try to read 3 or 4 bytes before getting back to the beggining
of the loop, so we're using the same condition here, which helps the
compiler perform the test only once and use unconditional branches from
there.

The code is unchanged on x86_64 and armv8 (out of ifdef) and no
measurable change is observed on other archs.

This should be backported to 1.2. The patch is easier consulted with
git show -b.

This is libslz upstream commit 4ff4b66c804629089c0bb16f141a6320f92eba10.

Note: the impact in haproxy is practically inexistent since we build by
      default with the tag at the end of pools, even if an extra byte
      were to be accessed on slower architectures, it would have no
      effect. Better backport it to avoid triggering ASAN though.

diff --git a/src/slz.c b/src/slz.c
index 2d670f83cbbc25fe2af57859e9cd62c6bf221821..08eb80de92fff7c8926b7701d7249d343d508269 100644 (file)
--- a/src/slz.c
+++ b/src/slz.c
@@ -542,7 +542,8 @@ long slz_rfc1951_encode(struct slz_stream *strm, unsigned char *out, const unsig
        strm->outbuf = out;
 
 #ifndef UNALIGNED_FASTER
-       word = ((unsigned char)in[pos] << 8) + ((unsigned char)in[pos + 1] << 16) + ((unsigned char)in[pos + 2] << 24);
+       if (rem >= 4)  // <word> is only used inside the loop below, hence the test for >= 4
+               word = ((unsigned char)in[pos] << 8) + ((unsigned char)in[pos + 1] << 16) + ((unsigned char)in[pos + 2] << 24);
 #endif
        while (rem >= 4) {
 #ifndef UNALIGNED_FASTER
@@ -726,11 +727,16 @@ long slz_rfc1951_encode(struct slz_stream *strm, unsigned char *out, const unsig
                pos += mlen;
 
 #ifndef UNALIGNED_FASTER
+               /* same as before the loop, this is only used when continuing
+                * in the loop, so let's use the same condition (rem>=4).
+                */
+               if (rem >= 4) {
 #ifdef UNALIGNED_LE_OK
-               word = *(uint32_t *)&in[pos - 1];
+                       word = *(uint32_t *)&in[pos - 1];
 #else
-               word = ((unsigned char)in[pos] << 8) + ((unsigned char)in[pos + 1] << 16) + ((unsigned char)in[pos + 2] << 24);
+                       word = ((unsigned char)in[pos] << 8) + ((unsigned char)in[pos + 1] << 16) + ((unsigned char)in[pos + 2] << 24);
 #endif
+               }
 #endif
        }