Re: [Qemu-devel] [PULL 3/3] contrib: add vhost-user-input

[Marc-André Lureau]

Hi

On Thu, May 30, 2019 at 1:17 PM Peter Maydell <address@hidden> wrote:
>
> On Wed, 22 May 2019 at 09:29, Gerd Hoffmann <address@hidden> wrote:
> >
> > From: Marc-André Lureau <address@hidden>
> >
> > Add a vhost-user input backend example, based on virtio-input-host
> > device. It takes an evdev path as argument, and can be associated with
> > a vhost-user-input device via a UNIX socket:
> >
> > $ vhost-user-input -p /dev/input/eventX -s /tmp/vui.sock
> >
> > $ qemu ... -chardev socket,id=vuic,path=/tmp/vui.sock
> >   -device vhost-user-input-pci,chardev=vuic
> >
> > This example is intentionally not included in $TOOLS, and not
> > installed by default.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > Message-id: address@hidden
> > Signed-off-by: Gerd Hoffmann <address@hidden>
>
> Hi; Coverity spotted a problem with this patch:
>
> > +int
> > +main(int argc, char *argv[])
> > +{
> > +    GMainLoop *loop = NULL;
> > +    VuInput vi = { 0, };
> > +    int rc, ver, fd;
> > +    virtio_input_config id;
> > +    struct input_id ids;
> > +    GError *error = NULL;
> > +    GOptionContext *context;
> > +
> > +    context = g_option_context_new(NULL);
> > +    g_option_context_add_main_entries(context, entries, NULL);
> > +    if (!g_option_context_parse(context, &argc, &argv, &error)) {
> > +        g_printerr("Option parsing failed: %s\n", error->message);
> > +        exit(EXIT_FAILURE);
> > +    }
> > +    if (opt_print_caps) {
> > +        g_print("{\n");
> > +        g_print("  \"type\": \"input\",\n");
> > +        g_print("  \"features\": [\n");
> > +        g_print("    \"evdev-path\",\n");
> > +        g_print("    \"no-grab\"\n");
> > +        g_print("  ]\n");
> > +        g_print("}\n");
> > +        exit(EXIT_SUCCESS);
> > +    }
> > +    if (!opt_evdev) {
> > +        g_printerr("Please specify an evdev path\n");
> > +        exit(EXIT_FAILURE);
> > +    }
> > +    if ((!!opt_socket_path + (opt_fdnum != -1)) != 1) {
> > +        g_printerr("Please specify either --fd or --socket-path\n");
> > +        exit(EXIT_FAILURE);
> > +    }
> > +
> > +    vi.evdevfd = open(opt_evdev, O_RDWR);
> > +    if (vi.evdevfd < 0) {
> > +        g_printerr("Failed to open evdev: %s\n", g_strerror(errno));
> > +        exit(EXIT_FAILURE);
> > +    }
> > +
> > +    rc = ioctl(vi.evdevfd, EVIOCGVERSION, &ver);
> > +    if (rc < 0) {
> > +        g_printerr("%s: is not an evdev device\n", argv[1]);
> > +        exit(EXIT_FAILURE);
> > +    }
> > +
> > +    if (!opt_nograb) {
> > +        rc = ioctl(vi.evdevfd, EVIOCGRAB, 1);
> > +        if (rc < 0) {
> > +            g_printerr("Failed to grab device\n");
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> > +
> > +    vi.config = g_array_new(false, false, sizeof(virtio_input_config));
> > +    memset(&id, 0, sizeof(id));
> > +    ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1), id.u.string);
>
> CID 1401704 -- we don't check the return value from ioctl().

ok, I'll send a fix

>
> > +    id.select = VIRTIO_INPUT_CFG_ID_NAME;
> > +    id.size = strlen(id.u.string);
> > +    g_array_append_val(vi.config, id);
> > +
> > +    if (ioctl(vi.evdevfd, EVIOCGID, &ids) == 0) {
> > +        memset(&id, 0, sizeof(id));
> > +        id.select = VIRTIO_INPUT_CFG_ID_DEVIDS;
> > +        id.size = sizeof(struct virtio_input_devids);
> > +        id.u.ids.bustype = cpu_to_le16(ids.bustype);
> > +        id.u.ids.vendor  = cpu_to_le16(ids.vendor);
> > +        id.u.ids.product = cpu_to_le16(ids.product);
> > +        id.u.ids.version = cpu_to_le16(ids.version);
> > +        g_array_append_val(vi.config, id);
> > +    }
> > +
> > +    vi_bits_config(&vi, EV_KEY, KEY_CNT);
> > +    vi_bits_config(&vi, EV_REL, REL_CNT);
> > +    vi_bits_config(&vi, EV_ABS, ABS_CNT);
> > +    vi_bits_config(&vi, EV_MSC, MSC_CNT);
> > +    vi_bits_config(&vi, EV_SW,  SW_CNT);
> > +    g_debug("config length: %u", vi.config->len);
> > +
> > +    if (opt_socket_path) {
> > +        int lsock = unix_listen(opt_socket_path, &error_fatal);
> > +        fd = accept(lsock, NULL, NULL);
> > +        close(lsock);
>
> This is CID 1401705 -- failure to check return value from
> unix_listen() -- which I just realised I probably replied
> to the wrong version of the patch to point out, so I mention
> it again here.

coverity should realize that passing &error_fatal will not return, no?

Can we mark this as false-positive?

>
> > +    } else {
> > +        fd = opt_fdnum;
> > +    }
>
> thanks
> -- PMM