s4:lsa RPC server - always initialise "info" structures

[Matthias Dieter Wallnöfer]

Jelmer and others,

the next patch initialises the missing field (reserved) - 
http://gitweb.samba.org/samba.git/?p=samba.git;a=commitdiff;h=70eaa3fd0cc1622724852039d63121a2c5ee22d1. 
Therefore no problem here!

Cheers,
Matthias

Jelmer Vernooij wrote:
> On Sat, 2010-12-04 at 11:10 +1100, Andrew Bartlett wrote:
>    
>> On Sat, 2010-12-04 at 00:18 +0100, Jelmer Vernooij wrote:
>>      
>>> Hi Matthias,
>>>
>>> On Fri, 2010-12-03 at 23:57 +0100, Matthias Dieter Wallnöfer wrote:
>>>        
>>>> commit d0b39324471e5226613a86aad313557cd4a89a9a
>>>> Author: Matthias Dieter Wallnöfer<mdw at samba.org>
>>>> Date:   Fri Dec 3 22:47:21 2010 +0100
>>>>
>>>>      s4:lsa RPC server - always initialise "info" structures
>>>>
>>>>      This should help to fix bug #7769
>>>>          
>>> I'm not sure if this (zeroing out the structs) is the right thing to do.
>>> The struct members that were previously uninitialised are now perhaps
>>> initialised - but set to zero. We should really be making sure that all
>>> members are set explicitly. Which ones were we missing here?
>>>
>>> One of the reasons we don't zero out variables sometimes is so valgrind
>>> will detect that we haven't fully initialised them when e.g. sending
>>> them over the wire.
>>>        
>> I have to say that I disagree, but thank you for bringing this up on the
>> list.
>>      
>    
>> I do remember when this was the was the rationale, but I've since
>> changed my view, mostly in response to talking to tridge (CC'ed) about
>> it.
>>      
>    
>> I've been convinced that for most things, 0 is an appropriate default,
>> and we run and analyse the output of valgrind so rarely over the whole
>> codebase that the benefits in simply not having these errors crop up in
>> unusual production settings outweigh the costs of not being able to
>> track these down with valgrind.
>>      
>    
>> That said, having found this particular case, it would be good to
>> validate that 0 is the right default here, before doing a blind
>> talloc_zero().
>>      
> Please note that I didn't say this commit was wrong, that it should be
> reverted or that talloc_zero is wrong. I merely pointed out that I
> wasn't sure that zeroing out this variable was necessarily the right
> thing to do and I feared that this code was being changed to use zero
> rather than checking the docs to see what the actually correct value
> would be.
>
> That said, I think in the case of DCE/RPC arguments like this we should
> generally aim to always set all variables explicitly rather than relying
> on zero'ing them out. Our callers expect these variables to be filled in
> since they will /always/ access them, serialize them and send them over
> the wire. And (barring 'reserved' members) there is no reason for
> leaving variables set to zero.
>
>    
>> As a good, separate, example see the cli_credentials_init():  It tried
>> to init the structure manually, but it's got quite silly with how many
>> initialisers there are, almost all to 0, and should just be a left
>>      
> with
>    
>> a talloc_zero().
>>      
> I think the credentials struct is a very different situation. Callers in
> that case are expected to only fill in only some of the member variables
> anyway, we can't require them to specify everything - and not all member
> variables will be used. The credentials struct is also prone to
> changes.
>
> Cheers,
>
> Jelmer
>