Re: Valgrind updates

[Linus Torvalds <torvalds@linux-foundation.org>]

On Tue, 27 Jan 2009, Johannes Schindelin wrote:
> 
> Come to think of it, the word "suppression" is probably a good indicator 
> that valgrind never claimed it would mark the zlib buffer as properly 
> initialized.

Hmm. The zlib faq has a note about zlib doing a conditional on 
uninitialized memory that doesn't matter, and that is what the suppression 
should be about (to avoid a warning about "Conditional jump or move 
depends on uninitialised value").

But that one is documented to not matter for the actual output (zlib 
FAQ#36).

It's possible that zlib really does leave padding bytes around that 
literally don't matter, and that don't get initialized. That really would 
be bad, because it means that the output of git wouldn't be repeatable. 
But I doubt this is the case - original git used to actually do the SHA1 
over the _compressed_ data, which was admittedly a totally and utterly 
broken design (and we fixed it), but it did work. Maybe it worked by luck, 
but I somehow doubt it.

Some googling did find this:

	http://mailman.few.vu.nl/pipermail/sysprog/2008-October/000298.html

which looks very similar: an uninitialized byte in the middle of a 
deflate() packet.

Anyway, I'm just going to Cc 'zlib@gzip.org', since this definitely is 
_not_ the same issue as in the FAQ, and we're not the only ones seeing it. 
For the zlib people: the code is literally this:

        /* Set it up */
        memset(&stream, 0, sizeof(stream));
        deflateInit(&stream, zlib_compression_level);
        size = 8 + deflateBound(&stream, len+hdrlen);
        compressed = xmalloc(size);

        /* Compress it */
        stream.next_out = compressed;
        stream.avail_out = size;

        /* First header.. */
        stream.next_in = (unsigned char *)hdr;
        stream.avail_in = hdrlen;
        while (deflate(&stream, 0) == Z_OK)
                /* nothing */;

        /* Then the data itself.. */
        stream.next_in = buf;
        stream.avail_in = len;
        ret = deflate(&stream, Z_FINISH);
        if (ret != Z_STREAM_END)
                die("unable to deflate new object %s (%d)", sha1_to_hex(sha1), ret);

        ret = deflateEnd(&stream);
        if (ret != Z_OK)
                die("deflateEnd on object %s failed (%d)", sha1_to_hex(sha1), ret);

        size = stream.total_out;

        if (write_buffer(fd, compressed, size) < 0)
                die("unable to write sha1 file");

and valgrind complains that the "write_buffer()" call will touch an 
uninitialized byte (just one byte, and in the _middle_ of the buffer, no 
less):

> Yet, the buffer in question is 195 bytes, stream.total_count (which 
> totally agrees with size - stream.avail_out) says it is 58 bytes, and 
> valgrind says that the byte with offset 51 is uninitialized.

The thing to note here is that what we are passing in to "write_buffer()" 
is _exactly_ what zlib deflated for us:

 - 'compressed' is the allocation, and is what we used to initialize 
   'stream.next_out' with (at the top of the code sequence above)

 - 'size' is gotten from 'stream.total_out' at the end of the compression.

Maybe the zlib people can tell us that we're idiots and the above is 
buggy, but maybe there is a real bug in zlib. Maybe it's triggered by our 
use of using two different input buffers to deflate() (ie we compress the 
header first, and then the body of the actual data, and put it all in one 
single output buffer), which may be unusual usage of zlib routines and may 
be why there aren't tons of reports of this.

(Our use of just depending on deflate() returning Z_BUF_ERROR after 
consuming all of the header data is probably also "unusual", but the 
manual explicitly says that it's not fatal and that deflate can be called 
again with more buffers).

Oh Gods of zlib, please hear our plea for clarification..

			Linus