[PATCH] s4-libcli: Fix comparsion of uninitalized bytes.

[simo]

On Tue, 2013-07-02 at 14:36 +0200, Volker Lendecke wrote:
> On Tue, Jul 02, 2013 at 12:23:48PM +0200, Andreas Schneider wrote:
> > Found by valgrind.
> > 
> > Signed-off-by: Andreas Schneider <asn at samba.org>
> > ---
> >  source4/libcli/clireadwrite.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/source4/libcli/clireadwrite.c b/source4/libcli/clireadwrite.c
> > index 7d8f34a..7d73e75 100644
> > --- a/source4/libcli/clireadwrite.c
> > +++ b/source4/libcli/clireadwrite.c
> > @@ -38,6 +38,8 @@ ssize_t smbcli_read(struct smbcli_tree *tree, int fnum, void *_buf, off_t offset
> >  		return 0;
> >  	}
> >  
> > +	memset(buf, 0, size);
> > +
> >  	parms.readx.level = RAW_READ_READX;
> >  	parms.readx.in.file.fnum = fnum;
> 
> To be honest, I don't understand why this should be
> necessary. I thought the idea of the smbcli_read call would
> be to download something from the server and fill it into
> "_buf". This would make the memory initialized. Why does
> valgrind require to first initialize something that we
> overwrite later anyway?
> 
> Don't get me wrong. I am not against fixing a valgrind bug.
> I just want to understand this.

I would guess valgrind may be complaining if thwere are some cases where
we allocate the buffer of size S, then read N < S bytes and when
returned the caller for some reason copies the full buffer or access any
bytes > S but < N. In that case the client would be accessing
uninitialized memory.

In test_session_reauth1() for example we compare buf to data, without
length checks, so if the code returned less then dlen bytes you'd get
valgrind errors about reading uninitialized data from the stack.

I think that we should check the size a not initialize buf
unconditionally to suppress valgrind warnings. It's a bug in the code
that we access data that is not initialized, so we should fix the source
of the issue instead.

Simo.