]> git.ipfire.org Git - thirdparty/zlib-ng.git/commitdiff
Fix a bug that can crash deflate on some input when using Z_FIXED.
authorMark Adler <madler@alumni.caltech.edu>
Wed, 18 Apr 2018 05:09:22 +0000 (22:09 -0700)
committerHans Kristian Rosbach <hk-github@circlestorm.org>
Wed, 14 Nov 2018 08:21:26 +0000 (09:21 +0100)
This bug was reported by Danilo Ramos of Eideticom, Inc. It has
lain in wait 13 years before being found! The bug was introduced
in zlib 1.2.2.2, with the addition of the Z_FIXED option. That
option forces the use of fixed Huffman codes. For rare inputs with
a large number of distant matches, the pending buffer into which
the compressed data is written can overwrite the distance symbol
table which it overlays. That results in corrupted output due to
invalid distances, and can result in out-of-bound accesses,
crashing the application.

The fix here combines the distance buffer and literal/length
buffers into a single symbol buffer. Now three bytes of pending
buffer space are opened up for each literal or length/distance
pair consumed, instead of the previous two bytes. This assures
that the pending buffer cannot overwrite the symbol table, since
the maximum fixed code compressed length/distance is 31 bits, and
since there are four bytes of pending space for every three bytes
of symbol space.

deflate.c
deflate.h
deflate_fast.c
deflate_medium.c
deflate_slow.c
trees.c

index 8d407cbd9075a4570dbcbc4c9b625f7f7fd1e778..b408ee8e3e5408c2cd0bf53915667424e7cbdba3 100644 (file)
--- a/deflate.c
+++ b/deflate.c
@@ -233,11 +233,6 @@ int ZEXPORT PREFIX(deflateInit2_)(PREFIX3(stream) *strm, int level, int method,
     int wrap = 1;
     static const char my_version[] = PREFIX2(VERSION);
 
-    uint16_t *overlay;
-    /* We overlay pending_buf and d_buf+l_buf. This works since the average
-     * output size for (length,distance) codes is <= 24 bits.
-     */
-
 #ifdef X86_CPUID
     x86_check_features();
 #endif
@@ -320,9 +315,47 @@ int ZEXPORT PREFIX(deflateInit2_)(PREFIX3(stream) *strm, int level, int method,
 
     s->lit_bufsize = 1 << (memLevel + 6); /* 16K elements by default */
 
-    overlay = (uint16_t *) ZALLOC(strm, s->lit_bufsize, sizeof(uint16_t)+2);
-    s->pending_buf = (unsigned char *) overlay;
-    s->pending_buf_size = (unsigned long)s->lit_bufsize * (sizeof(uint16_t)+2L);
+    /* We overlay pending_buf and sym_buf. This works since the average size
+     * for length/distance pairs over any compressed block is assured to be 31
+     * bits or less.
+     *
+     * Analysis: The longest fixed codes are a length code of 8 bits plus 5
+     * extra bits, for lengths 131 to 257. The longest fixed distance codes are
+     * 5 bits plus 13 extra bits, for distances 16385 to 32768. The longest
+     * possible fixed-codes length/distance pair is then 31 bits total.
+     *
+     * sym_buf starts one-fourth of the way into pending_buf. So there are
+     * three bytes in sym_buf for every four bytes in pending_buf. Each symbol
+     * in sym_buf is three bytes -- two for the distance and one for the
+     * literal/length. As each symbol is consumed, the pointer to the next
+     * sym_buf value to read moves forward three bytes. From that symbol, up to
+     * 31 bits are written to pending_buf. The closest the written pending_buf
+     * bits gets to the next sym_buf symbol to read is just before the last
+     * code is written. At that time, 31*(n-2) bits have been written, just
+     * after 24*(n-2) bits have been consumed from sym_buf. sym_buf starts at
+     * 8*n bits into pending_buf. (Note that the symbol buffer fills when n-1
+     * symbols are written.) The closest the writing gets to what is unread is
+     * then n+14 bits. Here n is lit_bufsize, which is 16384 by default, and
+     * can range from 128 to 32768.
+     *
+     * Therefore, at a minimum, there are 142 bits of space between what is
+     * written and what is read in the overlain buffers, so the symbols cannot
+     * be overwritten by the compressed data. That space is actually 139 bits,
+     * due to the three-bit fixed-code block header.
+     *
+     * That covers the case where either Z_FIXED is specified, forcing fixed
+     * codes, or when the use of fixed codes is chosen, because that choice
+     * results in a smaller compressed block than dynamic codes. That latter
+     * condition then assures that the above analysis also covers all dynamic
+     * blocks. A dynamic-code block will only be chosen to be emitted if it has
+     * fewer bits than a fixed-code block would for the same set of symbols.
+     * Therefore its average symbol length is assured to be less than 31. So
+     * the compressed data for a dynamic block also cannot overwrite the
+     * symbols from which it is being constructed.
+     */
+
+    s->pending_buf = (unsigned char *) ZALLOC(strm, s->lit_bufsize, 4);
+    s->pending_buf_size = (unsigned long)s->lit_bufsize * 4;
 
     if (s->window == NULL || s->prev == NULL || s->head == NULL ||
         s->pending_buf == NULL) {
@@ -331,8 +364,12 @@ int ZEXPORT PREFIX(deflateInit2_)(PREFIX3(stream) *strm, int level, int method,
         PREFIX(deflateEnd)(strm);
         return Z_MEM_ERROR;
     }
-    s->d_buf = overlay + s->lit_bufsize/sizeof(uint16_t);
-    s->l_buf = s->pending_buf + (1+sizeof(uint16_t))*s->lit_bufsize;
+    s->sym_buf = s->pending_buf + s->lit_bufsize;
+    s->sym_end = (s->lit_bufsize - 1) * 3;
+    /* We avoid equality with lit_bufsize*3 because of wraparound at 64K
+     * on 16 bit machines and because stored blocks are restricted to
+     * 64K-1 bytes.
+     */
 
     s->level = level;
     s->strategy = strategy;
@@ -516,7 +553,7 @@ int ZEXPORT PREFIX(deflatePrime)(PREFIX3(stream) *strm, int bits, int value) {
     if (deflateStateCheck(strm))
         return Z_STREAM_ERROR;
     s = strm->state;
-    if ((unsigned char *)(s->d_buf) < s->pending_out + ((Buf_size + 7) >> 3))
+    if (s->sym_buf < s->pending_out + ((Buf_size + 7) >> 3))
         return Z_BUF_ERROR;
     do {
         put = Buf_size - s->bi_valid;
@@ -1053,7 +1090,6 @@ int ZEXPORT PREFIX(deflateEnd)(PREFIX3(stream) *strm) {
 int ZEXPORT PREFIX(deflateCopy)(PREFIX3(stream) *dest, PREFIX3(stream) *source) {
     deflate_state *ds;
     deflate_state *ss;
-    uint16_t *overlay;
 
     if (deflateStateCheck(source) || dest == NULL) {
         return Z_STREAM_ERROR;
@@ -1073,8 +1109,7 @@ int ZEXPORT PREFIX(deflateCopy)(PREFIX3(stream) *dest, PREFIX3(stream) *source)
     ds->window = (unsigned char *) ZALLOC(dest, ds->w_size, 2*sizeof(unsigned char));
     ds->prev   = (Pos *)  ZALLOC(dest, ds->w_size, sizeof(Pos));
     ds->head   = (Pos *)  ZALLOC(dest, ds->hash_size, sizeof(Pos));
-    overlay = (uint16_t *) ZALLOC(dest, ds->lit_bufsize, sizeof(uint16_t)+2);
-    ds->pending_buf = (unsigned char *) overlay;
+    ds->pending_buf = (unsigned char *) ZALLOC(dest, ds->lit_bufsize, 4);
 
     if (ds->window == NULL || ds->prev == NULL || ds->head == NULL || ds->pending_buf == NULL) {
         PREFIX(deflateEnd)(dest);
@@ -1087,8 +1122,7 @@ int ZEXPORT PREFIX(deflateCopy)(PREFIX3(stream) *dest, PREFIX3(stream) *source)
     memcpy(ds->pending_buf, ss->pending_buf, (unsigned int)ds->pending_buf_size);
 
     ds->pending_out = ds->pending_buf + (ss->pending_out - ss->pending_buf);
-    ds->d_buf = overlay + ds->lit_bufsize/sizeof(uint16_t);
-    ds->l_buf = ds->pending_buf + (1+sizeof(uint16_t))*ds->lit_bufsize;
+    ds->sym_buf = ds->pending_buf + ds->lit_bufsize;
 
     ds->l_desc.dyn_tree = ds->dyn_ltree;
     ds->d_desc.dyn_tree = ds->dyn_dtree;
@@ -1555,7 +1589,7 @@ static block_state deflate_rle(deflate_state *s, int flush) {
         FLUSH_BLOCK(s, 1);
         return finish_done;
     }
-    if (s->last_lit)
+    if (s->sym_next)
         FLUSH_BLOCK(s, 0);
     return block_done;
 }
@@ -1592,7 +1626,7 @@ static block_state deflate_huff(deflate_state *s, int flush) {
         FLUSH_BLOCK(s, 1);
         return finish_done;
     }
-    if (s->last_lit)
+    if (s->sym_next)
         FLUSH_BLOCK(s, 0);
     return block_done;
 }
index 4ea906067cca1d9a7edb3459cfa1a9e370b6ba6b..81b9700827f6eb1d6d3d75852336773891ff8fa6 100644 (file)
--- a/deflate.h
+++ b/deflate.h
@@ -228,7 +228,7 @@ typedef struct internal_state {
     /* Depth of each subtree used as tie breaker for trees of equal frequency
      */
 
-    unsigned char *l_buf;       /* buffer for literals or lengths */
+    unsigned char *sym_buf;       /* buffer for distances and literals/lengths */
 
     unsigned int  lit_bufsize;
     /* Size of match buffer for literals/lengths.  There are 4 reasons for
@@ -250,13 +250,8 @@ typedef struct internal_state {
      *   - I can't count above 4
      */
 
-    unsigned int last_lit;        /* running index in l_buf */
-
-    uint16_t *d_buf;
-    /* Buffer for distances. To simplify the code, d_buf and l_buf have
-     * the same number of elements. To use different lengths, an extra flag
-     * array would be necessary.
-     */
+    unsigned int sym_next;      /* running index in sym_buf */
+    unsigned int sym_end;       /* symbol table full when sym_next reaches this */
 
     unsigned long opt_len;        /* bit length of current block with optimal trees */
     unsigned long static_len;     /* bit length of current block with static trees */
@@ -373,20 +368,22 @@ void ZLIB_INTERNAL bi_windup(deflate_state *s);
 
 # define _tr_tally_lit(s, c, flush) \
   { unsigned char cc = (c); \
-    s->d_buf[s->last_lit] = 0; \
-    s->l_buf[s->last_lit++] = cc; \
+    s->sym_buf[s->sym_next++] = 0; \
+    s->sym_buf[s->sym_next++] = 0; \
+    s->sym_buf[s->sym_next++] = cc; \
     s->dyn_ltree[cc].Freq++; \
-    flush = (s->last_lit == s->lit_bufsize-1); \
+    flush = (s->sym_next == s->sym_end); \
   }
 # define _tr_tally_dist(s, distance, length, flush) \
   { unsigned char len = (unsigned char)(length); \
     uint16_t dist = (uint16_t)(distance); \
-    s->d_buf[s->last_lit] = dist; \
-    s->l_buf[s->last_lit++] = len; \
+    s->sym_buf[s->sym_next++] = dist; \
+    s->sym_buf[s->sym_next++] = dist >> 8; \
+    s->sym_buf[s->sym_next++] = len; \
     dist--; \
     s->dyn_ltree[_length_code[len]+LITERALS+1].Freq++; \
     s->dyn_dtree[d_code(dist)].Freq++; \
-    flush = (s->last_lit == s->lit_bufsize-1); \
+    flush = (s->sym_next == s->sym_end); \
   }
 #else
 #   define _tr_tally_lit(s, c, flush) flush = _tr_tally(s, 0, c)
index 7c74dbe92519ac6c83d404b13ec9c7a20cedfe2e..a5861fe61e7a014af9b6f73dc07e98ef18c1c7fa 100644 (file)
@@ -114,7 +114,7 @@ ZLIB_INTERNAL block_state deflate_fast(deflate_state *s, int flush) {
         FLUSH_BLOCK(s, 1);
         return finish_done;
     }
-    if (s->last_lit)
+    if (s->sym_next)
         FLUSH_BLOCK(s, 0);
     return block_done;
 }
index 6b8dfc6da47c60a719c1c0ea0d06e1202075df4d..1b947287bfc2a2614a9fb923bd251e25b2450f17 100644 (file)
@@ -319,7 +319,7 @@ ZLIB_INTERNAL block_state deflate_medium(deflate_state *s, int flush) {
         FLUSH_BLOCK(s, 1);
         return finish_done;
     }
-    if (s->last_lit)
+    if (s->sym_next)
         FLUSH_BLOCK(s, 0);
 
     return block_done;
index de53b2df3a0a0039907d991bdba781e2429cbae9..1b2f70b56e006673cc6ec25aee879450906caf0f 100644 (file)
@@ -158,7 +158,7 @@ ZLIB_INTERNAL block_state deflate_slow(deflate_state *s, int flush) {
         FLUSH_BLOCK(s, 1);
         return finish_done;
     }
-    if (s->last_lit)
+    if (s->sym_next)
         FLUSH_BLOCK(s, 0);
     return block_done;
 }
diff --git a/trees.c b/trees.c
index 5a321b94f516e5eb4f8ef8384bea9b24f2e91128..c7e7760d71bb7baf647e765442a1a64d983edeca 100644 (file)
--- a/trees.c
+++ b/trees.c
@@ -337,7 +337,7 @@ static void init_block(deflate_state *s) {
 
     s->dyn_ltree[END_BLOCK].Freq = 1;
     s->opt_len = s->static_len = 0L;
-    s->last_lit = s->matches = 0;
+    s->sym_next = s->matches = 0;
 }
 
 #define SMALLEST 1
@@ -865,7 +865,8 @@ void ZLIB_INTERNAL _tr_flush_block(deflate_state *s, char *buf, unsigned long st
         static_lenb = (s->static_len+3+7) >> 3;
 
         Tracev((stderr, "\nopt %lu(%lu) stat %lu(%lu) stored %lu lit %u ",
-                opt_lenb, s->opt_len, static_lenb, s->static_len, stored_len, s->last_lit));
+                opt_lenb, s->opt_len, static_lenb, s->static_len, stored_len,
+                s->sym_next / 3));
 
         if (static_lenb <= opt_lenb)
             opt_lenb = static_lenb;
@@ -929,8 +930,9 @@ void ZLIB_INTERNAL _tr_flush_block(deflate_state *s, char *buf, unsigned long st
 int ZLIB_INTERNAL _tr_tally(deflate_state *s, unsigned dist, unsigned lc) {
     /* dist: distance of matched string */
     /* lc: match length-MIN_MATCH or unmatched char (if dist==0) */
-    s->d_buf[s->last_lit] = (uint16_t)dist;
-    s->l_buf[s->last_lit++] = (unsigned char)lc;
+    s->sym_buf[s->sym_next++] = dist;
+    s->sym_buf[s->sym_next++] = dist >> 8;
+    s->sym_buf[s->sym_next++] = lc;
     if (dist == 0) {
         /* lc is the unmatched char */
         s->dyn_ltree[lc].Freq++;
@@ -945,29 +947,7 @@ int ZLIB_INTERNAL _tr_tally(deflate_state *s, unsigned dist, unsigned lc) {
         s->dyn_ltree[_length_code[lc]+LITERALS+1].Freq++;
         s->dyn_dtree[d_code(dist)].Freq++;
     }
-
-#ifdef TRUNCATE_BLOCK
-    /* Try to guess if it is profitable to stop the current block here */
-    if ((s->last_lit & 0x1fff) == 0 && s->level > 2) {
-        /* Compute an upper bound for the compressed length */
-        unsigned long out_length = (unsigned long)s->last_lit*8L;
-        unsigned long in_length = (unsigned long)((long)s->strstart - s->block_start);
-        int dcode;
-        for (dcode = 0; dcode < D_CODES; dcode++) {
-            out_length += (unsigned long)s->dyn_dtree[dcode].Freq * (5L+extra_dbits[dcode]);
-        }
-        out_length >>= 3;
-        Tracev((stderr, "\nlast_lit %u, in %ld, out ~%ld(%ld%%) ",
-               s->last_lit, in_length, out_length, 100L - out_length*100L/in_length));
-        if (s->matches < s->last_lit/2 && out_length < in_length/2)
-            return 1;
-    }
-#endif
-    return (s->last_lit == s->lit_bufsize-1);
-    /* We avoid equality with lit_bufsize because of wraparound at 64K
-     * on 16 bit machines and because stored blocks are restricted to
-     * 64K-1 bytes.
-     */
+    return (s->sym_next == s->sym_end);
 }
 
 /* ===========================================================================
@@ -978,14 +958,15 @@ static void compress_block(deflate_state *s, const ct_data *ltree, const ct_data
     /* dtree: distance tree */
     unsigned dist;      /* distance of matched string */
     int lc;             /* match length or unmatched char (if dist == 0) */
-    unsigned lx = 0;    /* running index in l_buf */
+    unsigned sx = 0;    /* running index in sym_buf */
     int code;           /* the code to send */
     int extra;          /* number of extra bits to send */
 
-    if (s->last_lit != 0) {
+    if (s->sym_next != 0) {
         do {
-            dist = s->d_buf[lx];
-            lc = s->l_buf[lx++];
+            dist = s->sym_buf[sx++] & 0xff;
+            dist += (unsigned)(s->sym_buf[sx++] & 0xff) << 8;
+            lc = s->sym_buf[sx++];
             if (dist == 0) {
                 send_code(s, lc, ltree); /* send a literal byte */
                 Tracecv(isgraph(lc), (stderr, " '%c' ", lc));
@@ -1010,9 +991,9 @@ static void compress_block(deflate_state *s, const ct_data *ltree, const ct_data
                 }
             } /* literal or match pair ? */
 
-            /* Check that the overlay between pending_buf and d_buf+l_buf is ok: */
-            Assert((unsigned int)(s->pending) < s->lit_bufsize + 2*lx, "pendingBuf overflow");
-        } while (lx < s->last_lit);
+            /* Check that the overlay between pending_buf and sym_buf is ok: */
+            Assert(s->pending < s->lit_bufsize + sx, "pendingBuf overflow");
+        } while (sx < s->sym_next);
     }
 
     send_code(s, END_BLOCK, ltree);