[PATCH v2 0/3] migration: Fix possible access out of bounds

[PATCH v2 0/3] migration: Fix possible access out of bounds

[Fabiano Rosas]

Fix the issue detected by Coverity in format_time_str() and move
the function into the generic utils as suggested.

v2:
- Fix the incorrect loop condition.
- Make the array static and fix argument name in signature.

Fabiano Rosas (3):
  migration: HMP: Fix possible out-of-bounds access
  migration: HMP: Fix postcopy latency distribution label
  cutils: Add time_us_to_str

 include/qemu/cutils.h          |  1 +
 migration/migration-hmp-cmds.c | 19 ++-----------------
 util/cutils.c                  | 13 +++++++++++++
 3 files changed, 16 insertions(+), 17 deletions(-)

-- 
2.35.3

[PATCH v2 1/3] migration: HMP: Fix possible out-of-bounds access

[Fabiano Rosas]

Coverity has caught a bug in the formatting of time intervals for
postcopy latency distribution display in 'info migrate'.

While bounds checking the labels array, sizeof is incorrectly being
used. ARRAY_SIZE is the correct form of obtaining the size of an
array.

Fixes: 3345fb3b6d ("migration/postcopy: Add latency distribution report for blocktime")
Resolves: Coverity CID 1612248
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index cef5608210..bb954881d7 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -57,11 +57,9 @@ static const gchar *format_time_str(uint64_t us)
     const char *units[] = {"us", "ms", "sec"};
     int index = 0;
 
-    while (us > 1000) {
+    while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
         us /= 1000;
-        if (++index >= (sizeof(units) - 1)) {
-            break;
-        }
+        index++;
     }
 
     return g_strdup_printf("%"PRIu64" %s", us, units[index]);
-- 
2.35.3

[PATCH v2 2/3] migration: HMP: Fix postcopy latency distribution label

[Fabiano Rosas]

Fix the loop condition to avoid having a label with "1000 us" instead
of "1 ms".

Reported-by: Prasad Pandit <ppandit@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration-hmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index bb954881d7..a8b879c9d6 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -57,7 +57,7 @@ static const gchar *format_time_str(uint64_t us)
     const char *units[] = {"us", "ms", "sec"};
     int index = 0;
 
-    while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
+    while (us >= 1000 && index + 1 < ARRAY_SIZE(units)) {
         us /= 1000;
         index++;
     }
-- 
2.35.3

Re: [PATCH v2 2/3] migration: HMP: Fix postcopy latency distribution label

[Philippe Mathieu-Daudé]

On 16/7/25 20:26, Fabiano Rosas wrote:
> Fix the loop condition to avoid having a label with "1000 us" instead
> of "1 ms".
> 
> Reported-by: Prasad Pandit <ppandit@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   migration/migration-hmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Good catch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 3/3] cutils: Add time_us_to_str

[Fabiano Rosas]

The migration code has a function that converts a time value (us) to a
string with the proper suffix. Move it to cutils since it's generic
enough that it could be reused.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 include/qemu/cutils.h          |  1 +
 migration/migration-hmp-cmds.c | 17 ++---------------
 util/cutils.c                  | 13 +++++++++++++
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 36c68ce86c..0e8c5f4275 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -171,6 +171,7 @@ int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result);
 int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
 
 char *size_to_str(uint64_t val);
+char *time_us_to_str(uint64_t us);
 
 /**
  * freq_to_str:
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index a8b879c9d6..1706f3a0f7 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -52,19 +52,6 @@ static void migration_global_dump(Monitor *mon)
                    ms->clear_bitmap_shift);
 }
 
-static const gchar *format_time_str(uint64_t us)
-{
-    const char *units[] = {"us", "ms", "sec"};
-    int index = 0;
-
-    while (us >= 1000 && index + 1 < ARRAY_SIZE(units)) {
-        us /= 1000;
-        index++;
-    }
-
-    return g_strdup_printf("%"PRIu64" %s", us, units[index]);
-}
-
 static void migration_dump_blocktime(Monitor *mon, MigrationInfo *info)
 {
     if (info->has_postcopy_blocktime) {
@@ -121,8 +108,8 @@ static void migration_dump_blocktime(Monitor *mon, MigrationInfo *info)
         monitor_printf(mon, "Postcopy Latency Distribution:\n");
 
         while (item) {
-            g_autofree const gchar *from = format_time_str(1UL << count);
-            g_autofree const gchar *to = format_time_str(1UL << (count + 1));
+            g_autofree const gchar *from = time_us_to_str(1UL << count);
+            g_autofree const gchar *to = time_us_to_str(1UL << (count + 1));
 
             monitor_printf(mon, "  [ %8s - %8s ]: %10"PRIu64"\n",
                            from, to, item->value);
diff --git a/util/cutils.c b/util/cutils.c
index 9803f11a59..dd45d33173 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -1004,6 +1004,19 @@ char *freq_to_str(uint64_t freq_hz)
     return g_strdup_printf("%0.3g %sHz", freq, si_prefix(exp10));
 }
 
+char *time_us_to_str(uint64_t us)
+{
+    static const char *units[] = {"us", "ms", "sec"};
+    int index = 0;
+
+    while (us >= 1000 && index + 1 < ARRAY_SIZE(units)) {
+        us /= 1000;
+        index++;
+    }
+
+    return g_strdup_printf("%"PRIu64" %s", us, units[index]);
+}
+
 int qemu_pstrcmp0(const char **str1, const char **str2)
 {
     return g_strcmp0(*str1, *str2);
-- 
2.35.3

Re: [PATCH v2 0/3] migration: Fix possible access out of bounds

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Fix the issue detected by Coverity in format_time_str() and move
> the function into the generic utils as suggested.
>
> v2:
> - Fix the incorrect loop condition.
> - Make the array static and fix argument name in signature.
>
> Fabiano Rosas (3):
>   migration: HMP: Fix possible out-of-bounds access
>   migration: HMP: Fix postcopy latency distribution label
>   cutils: Add time_us_to_str
>
>  include/qemu/cutils.h          |  1 +
>  migration/migration-hmp-cmds.c | 19 ++-----------------
>  util/cutils.c                  | 13 +++++++++++++
>  3 files changed, 16 insertions(+), 17 deletions(-)

Queued patches 1 & 2.

Patch 3 can go along with Daniel's suggestion of unifying all the
helpers that add string labels to a number.