[v1] coverity fixes

[PATCH 03/12] util/filemonitor-inotify: qemu_file_monitor_watch(): avoid overflow

Posted by Vladimir Sementsov-Ogievskiy 1 year, 1 month ago

Prefer clear assertions instead of possible array overflow.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 util/filemonitor-inotify.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
index 2c45f7f176..09ef240174 100644
--- a/util/filemonitor-inotify.c
+++ b/util/filemonitor-inotify.c
@@ -81,16 +81,21 @@ static void qemu_file_monitor_watch(void *arg)
 
     /* 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);
+        const char *name;
+        QFileMonitorDir *dir;
+        uint32_t iev;
         int qev;
         gsize i;
+        struct inotify_event *ev = (struct inotify_event *)(buf + used);
+
+        assert(len - used >= sizeof(struct inotify_event));
+        assert(len - used - sizeof(struct inotify_event) >= ev->len);
+
+        name = ev->len ? ev->name : "";
+        dir = g_hash_table_lookup(mon->idmap, GINT_TO_POINTER(ev->wd));
+        iev = ev->mask &
+            (IN_CREATE | IN_MODIFY | IN_DELETE | IN_IGNORED |
+             IN_MOVED_TO | IN_MOVED_FROM | IN_ATTRIB);
 
         used += sizeof(struct inotify_event) + ev->len;
 
-- 
2.34.1

Re: [PATCH 03/12] util/filemonitor-inotify: qemu_file_monitor_watch(): avoid overflow

Posted by Peter Maydell 1 year, 1 month ago

On Mon, 25 Sept 2023 at 20:43, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Prefer clear assertions instead of possible array overflow.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  util/filemonitor-inotify.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
> index 2c45f7f176..09ef240174 100644
> --- a/util/filemonitor-inotify.c
> +++ b/util/filemonitor-inotify.c
> @@ -81,16 +81,21 @@ static void qemu_file_monitor_watch(void *arg)
>
>      /* 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);
> +        const char *name;
> +        QFileMonitorDir *dir;
> +        uint32_t iev;
>          int qev;
>          gsize i;
> +        struct inotify_event *ev = (struct inotify_event *)(buf + used);
> +
> +        assert(len - used >= sizeof(struct inotify_event));
> +        assert(len - used - sizeof(struct inotify_event) >= ev->len);

So this is something we can assert because we trust the kernel
(which is what's on the other end of the inotify fd) to only
give us a valid buffer with complete event records in it, not
partial pieces, right? If so, then we should say so in a comment.

(FWIW in the online Coverity Scan we just marked this one as a
false-positive.)

thanks
-- PMM