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@xxxxxxxx', 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
