[PATCH] connection: Don't add uninitialized memory as 4 byte alignment padding

[Jonas Ådahl]

On Wed, Feb 10, 2016 at 12:37:43PM -0600, Derek Foreman wrote:
> On 10/02/16 09:35 AM, Jonas Ådahl wrote:
> > When we are adding padding bytes making our wl_buffer buffer content 4
> > byte aligned, we are just moving the pointer. Since the buffer is
> > allocated using plain malloc(), this means our padding bytes are
> > effectively uninitialized data, which could be anything previously
> > allocated in the server process. As we'll be sharing this buffer
> > content with arbitrary clients, we are effectively sharing private
> > memory with every client, and even though a well behaving client will
> > discard any such memory, a malicious client may not.
> > 
> > Therefor, to avoid any potential missuse of the uninitialized padding
> > memory shared between the server and client, initialize the buffer
> > content to 0, making the padding bytes always 0.
> 
> Cool - is this the cause of the valgrind complaints I've never bothered
> to track down? :)

Yes. Should have mentioned that in the commit message, as well as the
bug (https://bugs.freedesktop.org/show_bug.cgi?id=94071 ) that made me
actually dig out what memory was uninitialized. I'll add those two
things to the commit message.

> 
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >  src/connection.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/connection.c b/src/connection.c
> > index 65b64e9..c0e322f 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -1137,7 +1137,7 @@ wl_closure_send(struct wl_closure *closure, struct wl_connection *connection)
> >  		return -1;
> >  
> >  	buffer_size = buffer_size_for_closure(closure);
> > -	buffer = malloc(buffer_size * sizeof buffer[0]);
> > +	buffer = zalloc(buffer_size * sizeof buffer[0]);
> 
> Oh, took me a couple of reads to realize the padding is between certain
> potential members in the closure. (and not trivially at the start or end
> of the buffer)
> 
> I guess we could zero just the pad bytes in serialize_closure, I don't
> know whether that falls under "premature optimization" or not, though.
> Looks pretty easy to do...

We could do that, but is calloc() really any noticably slower than
malloc() in the way we are using it?

It is fairly easy (I did it at first just to make sure it was those
bytes only that triggered the warning), but s/malloc/zalloc/ seemed like
the cleaner solution to me.


Jonas

> 
> In any case,
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> 
> Thanks,
> Derek
> 
> >  	if (buffer == NULL)
> >  		return -1;
> >  
> > 
>