Re: [Qemu-devel] [PATCH v2 2/5] net: fix -netdev socket, fd= for UDP sockets

[Peter Maydell]

On 8 August 2017 at 21:38, Jens Freimann <jfreim...@redhat.com> wrote:
> This patch fixes -netdev socket,fd= for UDP sockets
> Currently -netdev socket,fd=<...> results in
>
>   qemu: error: specified mcastaddr "127.0.0.1" (0x7f000001) does not
>     contain a multicast address
>   qemu-system-x86_64: -netdev
>     socket,id=n1,fd=3: Device 'socket' could not be initialized
>
> To fix these we need to allow specifying multicast and fd arguments
> for the same netdev. With this the user can specify "-netdev
> fd=3,mcast=<IP:port>"
>
> Cc: Jason Wang <jasow...@redhat.com>
> Fixes: 3d830459b1eccdb61b75e2712fd364012ce5a115
> Signed-off-by: Jens Freimann <jfreim...@redhat.com>
> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>

Hi. It looks like this patch (commit 0f8c289ad539 in master)
introduced a coverity issue (CID1005339):

> @@ -333,8 +333,13 @@ static NetSocketState 
> *net_socket_fd_init_dgram(NetClientState *peer,
>       * by ONLY ONE process: we must "clone" this dgram socket --jjo
>       */
>
> -    if (is_connected) {
> -        if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) {
> +    if (is_connected && mcast != NULL) {

This changes the condition() under which we fill in the struct sockaddr_in saddr
from "if (is_connected)" to "if (is_connected && mcast != NULL)"...

> +            if (parse_host_port(&saddr, mcast) < 0) {
> +                fprintf(stderr,
> +                        "qemu: error: init_dgram: fd=%d failed 
> parse_host_port()\n",
> +                        fd);
> +                goto err;
> +            }
>              /* must be bound */
>              if (saddr.sin_addr.s_addr == 0) {
>                  fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "

...but later in the function we do:

    /* mcast: save bound address as dst */
    if (is_connected) {
        s->dgram_dst = saddr;
        snprintf(nc->info_str, sizeof(nc->info_str),
                 "socket: fd=%d (cloned mcast=%s:%d)",
                 fd, inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
    } else {
        snprintf(nc->info_str, sizeof(nc->info_str),
                 "socket: fd=%d", fd);
    }

and coverity correctly points out that if is_connected is true
but mcast is NULL then we use 'saddr' without having initialized
it properly.

Any suggestions for the correct fix for this?

thanks
-- PMM