[Zlib-devel] [PATCH] One remaining Valgrind error

[Jan Seiffert]

Daniel Richard G. schrieb:
> On Mon, 13 Aug 2012, Mark Adler wrote:
> 
>> Thank you for the patch, but I will definitely not be applying it.
>> That's way too much overhead to add to every deflateInit call.
> 
> How is it too much overhead? This is in an initialization routine (as
> opposed to an inner loop) that includes four calls to
> ZALLOC()/malloc(). deflateInit2_(), and thus this new
> zmemzero()/memset() call, get called all of fourteen times in "make
> check".
> 

As i told you already, it's the "inner loop" outside of zlib.
Every compressed http-connection (the majority today) needs a
new zstream (or an inflate reset). You are not just memsetting
some 100 bytes, but prop. 64k Byte.
So $ALEXA_WEB_TOP_10 will send a patch to remove it because their
server handle less load/need more energy. For them it's a regression.

>> In any case, I think valgrind is wrong.  What is valgrind
>> reporting?
> 
> Here is an example of what I typically see. This is from a Valgrind
> run on LibXML2's runtest program:
> 
> Conditional jump or move depends on uninitialised value(s)
>  ==631== at 0x5056510: inflateReset2 (in /lib/x86_64-linux-gnu/libz.so.1.2.3.4)

And here we have the little bugger.
But your patch was for deflateInit2_
So call me confused.

The only thing i see in infalteReset2 that may case this is that this:

if (state->window != Z_NULL && state->wbits != (unsigned)windowBits)

short circuit evaluation is not 100% honored by the compiler, nothing
wrong is generated, but it confuses valgrind enough (for example, the
compiler schedules the fetch early, but never really uses the value).

Please disassemble (objdump -d) the following routines and send the
result:
inflateInit2_
inflateReset2
inflateReset
inflateResetKeep

> 
> Note that Valgrind indicates no other memory issues.
> 

This is really the right error trace and nothing with deflate in it?

> This kind of thing is frustrating in general development work because
> the zlib errors are completely extraneous. If I could count the times
> that I've gone, "Crap, there's a memory error in my---oh, wait, never
> mind, that's just zlib noise. Somebody ought to fix that."
> 
> And while Valgrind may be wrong about the uninitialised value(s)
> being a problem, zlib doesn't have the luxury of being treated
> specially. Between the effort involved in teaching every memory
> debugger in existence "hey, don't worry, zlib knows what it's doing
> here,"

Why not? Enough libs are treated that way, esp. because these debugger are
not perfect.

> and applying a one-line patch that doesn't affect performance
> in any meaningful way...

And here you are wrong, sorry.
For some clients it's an performance path. If the memset would be smaller,
say the first 16 bytes of the array or something like that...

> I think the patch is preferable.
> 
> 
> --Daniel
> 
> 

Greetings
	Jan

-- 
const int TEN=11; /* Mine goes to eleven */