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

[Eric Blake]

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!).

> +    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.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature