s4:lsa RPC server - always initialise "info" structures
Matthias Dieter Wallnöfer
mdw at samba.org
Sat Dec 4 01:34:29 MST 2010
Jelmer and others,
the next patch initialises the missing field (reserved) -
Therefore no problem here!
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
>> 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
>> 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
> 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
>> 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
More information about the samba-technical