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

[Jelmer Vernooij]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101204/03acfc92/attachment.pgp>