Re: [Qemu-devel] [PATCH 2/2] io: fix stack allocation when sending of file descriptors

[Daniel P. Berrange]

On Tue, Dec 22, 2015 at 11:20:30AM -0700, Eric Blake wrote:
> On 12/21/2015 09:23 AM, Daniel P. Berrange wrote:
> > When sending file descriptors over a socket, we have to
> > allocate a data buffer to hold the FDs in the scmsghdr.
> > Unfortunately we allocated the buffer on the stack inside
> > an if () {} block, but called sendmsg() outside the block.
> > So the stack bytes holding the FDs were liable to be
> > overwritten with other data. By luck this was not a problem
> > when sending 1 FD, but if sending 2 or more then it would
> > fail.
> > 
> > The fix is to simply move the variables outside the nested
> > 'if' block. To keep valgrind quiet we also zero-initialize
> > the 'control' buffer.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  io/channel-socket.c            |  7 ++-
> >  tests/test-io-channel-socket.c | 98 
> > ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 101 insertions(+), 4 deletions(-)
> > 
> 
> The fix itself is obvious from the commit message; the bulk of this
> patch is the testsuite addition (which is a GOOD thing - thanks!).

Yes, I wasted lots of time trying to find the flaw before
I wrote the test case at which point it was trivial to
find with valgrind :-)

> 
> > +    qio_channel_readv_full(dst,
> > +                           iorecv,
> > +                           G_N_ELEMENTS(iorecv),
> > +                           &fdrecv,
> > +                           &nfdrecv,
> > +                           &error_abort);
> > +
> > +    g_assert(nfdrecv == G_N_ELEMENTS(fdsend));
> > +    /* Each recvd FD should be different from sent FD */
> > +    for (i = 0; i < nfdrecv; i++) {
> > +        g_assert_cmpint(fdrecv[i], !=, testfd);
> > +    }
> 
> Here, you blindly dereference fdrecv[]...
> 
> > +    unlink(TEST_FILE);
> > +    close(testfd);
> > +    if (fdrecv != NULL) {
> 
> ...so this if() is dead, and you can just always do the cleanup.

Yep, will fix

> That's minor, so:
> Reviewed-by: Eric Blake <address@hidden>

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|