Re: [Qemu-devel] [PATCH v8 01/11] util: add helper APIs for dealing with inotify in portable manner

[Peter Maydell]

On Fri, 15 Feb 2019 at 16:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The inotify userspace API for reading events is quite horrible, so it is
> useful to wrap it in a more friendly API to avoid duplicating code
> across many users in QEMU. Wrapping it also allows introduction of a
> platform portability layer, so that we can add impls for non-Linux based
> equivalents in future.

Hi; Coverity has suddenly decided to complain about this 3-year-old
code (in CID 1469132). It reports an "untrusted loop bound" because
in the 'loop over events in the buffer' we use the data we just read
from the filedescriptor (specifically ev->len) as part of the
calculation of our loop termination condition.

Is there actually anything to change here, or is this a false
positive because we actually trust the data we're getting on this fd?

(More generally there are a handful of "untrusted loop bound"
and "untrusted value as argument" issues outstanding in the Coverity UI,
so some opinions on whether they're valid or not would be useful.)

> +static void qemu_file_monitor_watch(void *arg)
> +{
> +    QFileMonitor *mon = arg;
> +    char buf[4096]
> +        __attribute__ ((aligned(__alignof__(struct inotify_event))));
> +    int used = 0;
> +    int len;
> +
> +    qemu_mutex_lock(&mon->lock);
> +
> +    if (mon->fd == -1) {
> +        qemu_mutex_unlock(&mon->lock);
> +        return;
> +    }
> +
> +    len = read(mon->fd, buf, sizeof(buf));
> +
> +    if (len < 0) {
> +        if (errno != EAGAIN) {
> +            error_report("Failure monitoring inotify FD '%s',"
> +                         "disabling events", strerror(errno));
> +            goto cleanup;
> +        }
> +
> +        /* no more events right now */
> +        goto cleanup;
> +    }
> +
> +    /* Loop over all events in the buffer */
> +    while (used < len) {
> +        struct inotify_event *ev =
> +            (struct inotify_event *)(buf + used);
> +        const char *name = ev->len ? ev->name : "";
> +        QFileMonitorDir *dir = g_hash_table_lookup(mon->idmap,
> +                                                   GINT_TO_POINTER(ev->wd));
> +        uint32_t iev = ev->mask &
> +            (IN_CREATE | IN_MODIFY | IN_DELETE | IN_IGNORED |
> +             IN_MOVED_TO | IN_MOVED_FROM | IN_ATTRIB);
> +        int qev;
> +        gsize i;
> +
> +        used += sizeof(struct inotify_event) + ev->len;
> +
> +        if (!dir) {
> +            continue;
> +        }
> +
> +        /*
> +         * During a rename operation, the old name gets
> +         * IN_MOVED_FROM and the new name gets IN_MOVED_TO.
> +         * To simplify life for callers, we turn these into
> +         * DELETED and CREATED events
> +         */
> +        switch (iev) {
> +        case IN_CREATE:
> +        case IN_MOVED_TO:
> +            qev = QFILE_MONITOR_EVENT_CREATED;
> +            break;
> +        case IN_MODIFY:
> +            qev = QFILE_MONITOR_EVENT_MODIFIED;
> +            break;
> +        case IN_DELETE:
> +        case IN_MOVED_FROM:
> +            qev = QFILE_MONITOR_EVENT_DELETED;
> +            break;
> +        case IN_ATTRIB:
> +            qev = QFILE_MONITOR_EVENT_ATTRIBUTES;
> +            break;
> +        case IN_IGNORED:
> +            qev = QFILE_MONITOR_EVENT_IGNORED;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +
> +        trace_qemu_file_monitor_event(mon, dir->path, name, ev->mask, 
> dir->id);
> +        for (i = 0; i < dir->watches->len; i++) {
> +            QFileMonitorWatch *watch = &g_array_index(dir->watches,
> +                                                      QFileMonitorWatch,
> +                                                      i);
> +
> +            if (watch->filename == NULL ||
> +                (name && g_str_equal(watch->filename, name))) {
> +                trace_qemu_file_monitor_dispatch(mon, dir->path, name,
> +                                                 qev, watch->cb,
> +                                                 watch->opaque, watch->id);
> +                watch->cb(watch->id, qev, name, watch->opaque);
> +            }
> +        }
> +    }
> +
> + cleanup:
> +    qemu_mutex_unlock(&mon->lock);
> +}
> +

thanks
-- PMM