From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34636) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBh0d-0003NN-8T for qemu-devel@nongnu.org; Wed, 23 Dec 2015 05:51:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBh0Z-0000Ya-HF for qemu-devel@nongnu.org; Wed, 23 Dec 2015 05:51:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37009) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBh0Z-0000YN-BQ for qemu-devel@nongnu.org; Wed, 23 Dec 2015 05:51:03 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id E9CE0C00358F for ; Wed, 23 Dec 2015 10:51:02 +0000 (UTC) Date: Wed, 23 Dec 2015 10:50:59 +0000 From: "Daniel P. Berrange" Message-ID: <20151223105059.GD20028@redhat.com> References: <1450715016-18230-1-git-send-email-berrange@redhat.com> <1450715016-18230-3-git-send-email-berrange@redhat.com> <5679946E.2060700@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5679946E.2060700@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2] io: fix stack allocation when sending of file descriptors Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org 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 > > --- > > 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 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 :|