Dr. Mark Adler on ZLIB memory leaks

[TO...@aol.com]

Hello all...
Kevin Kiley here..

I spent the day talking to Dr. Mark Adler ( co-author of ZLIB )
with the specific focus being certain issues that are on the
table here at the Apache forum.

As Peter (Cranstone) and I mentioned in the exchange last week 
( but things got lost in the noise ) there are certain issues with regards 
to using ZLIB as a 24x7 dynamic data compressor that need to be 
considered before anything that requires its use gets bombed into 
the core distribution.

The decision is ultimately yours but wanted you to have as much
information as possible.

This first message covers the 'memory leak' issue(s).

There will be another message following this one with
Mark's comments on ZLIB thread-safeness and
the legal/patent issues.

We, ourselves, were not sure last weekend what the final
upshot was on some of the questions raised because 
mod_gzip 1.3.19.1a for Apache 1.x.x series does NOT use
ZLIB so we thought we would front-end the questions to
Mark on behalf of the Apache group.

BTW: Mark is more than welcome to come here and talk about
this if you guys want/need him to but after you read the
text of today's conversation(s) I don't think it will be
necessary.

SUMMARY: 

1. Mark is not aware of any memory leaks in the
deflate aspect of ZLIB unless the cleanup call(s) 
are somehow 'missed' and/or the application using ZLIB
is not truly 'thread-safe' but inflate definitely has some.
He has private patches for this which are NOT in
any standard distribution of ZLIB. ( See below ).

2. If Apache is going to include ZLIB then it SHOULD be
a SOURCE LEVEL distribution and Apache should NOT depend
on just using whatever is on someone's machine. He explains
why below.


* THE CONVERSATIONS...

One of the topics of conversation this afternoon concerned
the 'memory leak' issues in ZLIB...

CONVERSATION 1...

At 12:38 AM -0500 9/8/01 Kevin Kiley and Mark Adler had
the following conversation...

>> Kevin Kiley asked...
>>
>> What do you know about memory leaks in the current version(s)
>> of ZLIB that are out there? Our own testing seems to
>> show there is a possiblity of leakage when running ZLIB
>> as a 24x7 dynamic data compression engine.
>> Do you know of any to speak of?
>
> Mark Adler wrote...
>
> No, not with deflate. I do know of memory leaks in inflate on some 
> error returns, and I have patches for those.
>
>> Kevin Kiley wrote...
>>
>> Is ZLIB absolutely known to have NO memory leaks or is there still
>> a chance that on a 24x7 level operation level it just might?
>
> Mark Adler wrote...
>
> There is a chance. But deflate has very simple error returns 
> considering that there's not much that can go wrong with deflate 
> since all input data is valid.  Since this is important to you, I 
> will manually check deflate for memory leaks as well as run some 
> tests.
> 
> This probably goes without saying, but if the application neglects to 
> call deflateEnd to deallocate the stream, then of course there will 
> be a big, fat memory leak.

END OF CONVERSATION 1


At this point Mark looked into this some more and did his own
testing in California for me.

A few hours or so later Mark Adler came back with this...


CONVERSATION 2...

At 5:17 PM - 9/8/01 Mark Adler posted the following...

Mark Adler wrote...

Well after an thorough manual examination of deflateInit, deflate, 
and deflateEnd and all functions called by those, it is clear to me 
that the only way to get a memory leak is to use deflate improperly, 
i.e. to not call deflateEnd when you need to, or to have a corrupted 
deflate state where the pointers or status have been overwritten 
somehow before calling deflateEnd.

In other words, zlib's deflateInit/deflate/deflateEnd has no memory leaks.

The memory allocation in deflate is very straightforward.  It is 
simply this in pseudo code:

deflateInit:
deflateInit2:
   if (compile time zlib.h doesn't match linked code) then
     return Z_VERSION_ERROR
   if (null stream supplied or bad parameters) then
     return Z_STREAM_ERROR
   s = calloc(statesize)         // note that calloc makes all pointers NULL
   if (s is NULL) then return Z_MEM_ERROR
   s.window = calloc(windowsize)
   s.prev = calloc(prevsize)
   s.head = calloc(headsize)
   s.pending_buf = calloc(bufsize)
   if (any of s.window or s.prev or s.head or s.pending_buf are NULL) then
     deflateEnd(s)
     return Z_MEM_ERROR
   otherwise return Z_OK

deflateEnd:
   if (status is not one of INIT_STATE or BUSY_STATE or FINISH_STATE) then
     return Z_STREAM_ERROR;      // intended to detect corruption of state --
                                 // there are only three possible states
   if (s.pending_buf is not NULL) then free(s.pending_buf)
   if (s.head is not NULL) then free(s.head)
   if (s.prev is not NULL) then free(s.prev)
   if (s.window is not NULL) then free(s.window)
   free(s)
   return Z_OK

>From that, some simple rules:

1. If deflateInit or deflateInit2 returns any error (Z_VERSION_ERROR, 
Z_STREAM_ERROR, or Z_MEM_ERROR), then no memory was allocated and 
deflateEnd should not be called.

2. If deflateInit or deflateInit2 returns Z_OK, then the application 
must eventually call deflateEnd with the constructed (uncorrupted) 
state in order to free the allocated memory.

3. The above is true no matter what conditions are returned by 
subsequent calls of deflate.  deflate itself does no allocating or 
freeing.

4. If deflateEnd returns Z_STREAM_ERROR, then the stream was 
corrupted by some external force and deflateEnd detected that it was 
corrupted.  In that case, deflateEnd does not try to free the state, 
since the pointers are most likely invalid.

5. It is possible for the stream to be corrupted by the evil external 
force, and for deflateEnd to not detect it.  In that case, free will 
likely be called with invalid pointers, and there is no predicting 
what will happen then.  Most C memory allocators are not well-behaved 
when asked to free an invalid pointer.

6. If a custom memory allocator is provided to zlib, then the 
allocator must set the contents of the allocated memory to zeros, 
like calloc.  This assumes that NULL is zero.  (Are there any 
machines where NULL is not zero?)

7. If the application requires zlib to be thread-safe, then the 
provided or default allocation routines must also be thread-safe. 
The default Unix allocators used are calloc() and free() from 
stdlib.h.

mark


CONVERSATION 3...

At 7:34 PM -0500 9/8/01, Kevin Kiley and Mark Adler
had the following exchange...

>> Kevin Kiley wrote...
>>
>> You said earlier today that you know of some memory
>> leaks in even the latest version of inflate but that you
>> has simple some patches for that?
>
> Mark Adler wrote...
>
> The patch to inflate ( to fix memory leaks ) is below.  (Yes, we've had 
> this patch for over two years but haven't gotten around to releasing 
> a new version of zlib.)
>
>> Kevin Kiley wrote...
>>
>> In light of the patches that need to be applied then I guess
>> the only sane thing to do is for Apache to just make sure they
>> include a SOURCE LEVEL distribution of ZLIB in their Server
>> so it can be the 'latest and greatest' at all times. Many
>> people have ZLIB but in order to make sure Apache users have
>> the best and safest ZLIB do you think it would be wise to always 
>> build and link to self-built up-to-date versions of the
>> ZLIB libraries in Apache's own source tree?
>
> Mark Adler wrote...
>
> I agree.  I think it's a lot easier to control the configuration, 
> compilation options, and linkage if it is included as part of the 
> Apache source and as part of the Apache make.  While it is true that 
> zlib is already installed as a shared library on many machines, zlib 
> is small enough that it is should not be a significant burden to have 
> a few redundant zlib's floating around on a high-powered server.
> 
> mark
>
> Here are the patches to run against infblock.c in the ZLIB
> source distribution that fix the memory leaks...

*** infblock.c.113      Mon Jun  8 10:06:16 1998
--- infblock.c  Sun Mar  7 15:27:48 1999
***************
*** 249,258 ****
                                &s->sub.trees.tb, s->hufts, z);
         if (t != Z_OK)
         {
-         ZFREE(z, s->sub.trees.blens);
           r = t;
           if (r == Z_DATA_ERROR)
             s->mode = BAD;
           LEAVE
         }
         s->sub.trees.index = 0;
--- 249,260 ----
                                &s->sub.trees.tb, s->hufts, z);
         if (t != Z_OK)
         {
           r = t;
           if (r == Z_DATA_ERROR)
+         {
+           ZFREE(z, s->sub.trees.blens);
             s->mode = BAD;
+         }
           LEAVE
         }
         s->sub.trees.index = 0;
***************
*** 313,323 ****
           t = inflate_trees_dynamic(257 + (t & 0x1f), 1 + ((t >> 5) & 0x1f),
                                     s->sub.trees.blens, &bl, &bd, &tl, &td,
                                     s->hufts, z);
-         ZFREE(z, s->sub.trees.blens);
           if (t != Z_OK)
           {
             if (t == (uInt)Z_DATA_ERROR)
               s->mode = BAD;
             r = t;
             LEAVE
           }
--- 315,327 ----
           t = inflate_trees_dynamic(257 + (t & 0x1f), 1 + ((t >> 5) & 0x1f),
                                     s->sub.trees.blens, &bl, &bd, &tl, &td,
                                     s->hufts, z);
           if (t != Z_OK)
           {
             if (t == (uInt)Z_DATA_ERROR)
+           {
+             ZFREE(z, s->sub.trees.blens);
               s->mode = BAD;
+           }
             r = t;
             LEAVE
           }
***************
*** 329,334 ****
--- 333,339 ----
           }
           s->sub.decode.codes = c;
         }
+       ZFREE(z, s->sub.trees.blens);
         s->mode = CODES;
       case CODES:
         UPDATE