[squid-dev] [PATCH] Uninitialised errors during Squid startup

[Amos Jeffries]

On 10/06/2016 1:36 a.m., Alex Rousskov wrote:
> On 06/08/2016 10:27 PM, Amos Jeffries wrote:
>> On 9/06/2016 12:52 a.m., Eduard Bagdasaryan wrote:
>>> Hello,
>>>
>>> This patch fixes valgrind-discovered trunk errors.
>>>
>>> During start-up, Valgrind reported many errors with a similar message:
>>> "Use of uninitialised value of size 8...". These errors were caused by
>>> HttpRequestMethod& parameter during private key generation:
>>> it was used as a raw void* memory there. HttpRequestMethod is a non-POD
>>> type and has some "padding" bytes, which are not initialized by the
>>> constructor.
>>>
>>> The patch simplifies private key generation by using a counter for this,
>>> which is sufficient to create a unique key within a single Squid process.
>>> Except fixing Valgrind errors, this solution saves many CPU cycles
>>> wasted on
>>> generating MD5 hashes just to "guarantee" uniqueness within a single
>>> process.
> 
>> How does this interact with objects that begin their life as private
>> then get converted to public keys?
> 
> This change is not meant to affect public keys.
> 
> 
>> * the comment about kidID and PID in the new private key blob as being
>> just an optimization I dont think is correct. Without them I think we
>> could have ID collisions in collapsed_forwarding, shared cache_mem, or
>> rock across workers.
> 
> I believe the comment is correct because private keys are not shared
> among kids. Being unknown to others is the essence of being a private
> key. The only entities that are supposed to know about a private key is
> the entity holding the corresponding StoreEntry lock. One cannot find a
> StoreEntry with a private key in Store.
> 
> If you know of any exceptions to that rule, please let me know.
> 
> 
>> That would make them required for proper operations in SMP.
> 
> Caching (SMP-aware and not) should be done using public, not private keys.
> 
> It may help to look at this bug from a different angle: After we stopped
> zeroing memory and before this fix, all private keys were essentially
> random values -- calling private key generation function with the same
> set of arguments would produce a different value. And yet, everything
> "worked" because private keys are compatible with random values by their
> nature/definition. The fix takes advantage of that understanding to
> simplify and optimize private key generation.
> 

Aha. Thank you.

+1 then from me. Unless you get to it applying it first I'll apply once
I'm done with the latest configure fixes.

Amos