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

[PATCH 0/2] 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.

Fabiano Rosas (2):
  migration: Fix postcopy latency distribution formatting computation
  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 1/2] migration: Fix postcopy latency distribution formatting computation

[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>
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 2/2] 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>
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..7621726621 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 val);
 
 /**
  * freq_to_str:
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index bb954881d7..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..023793211a 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)
+{
+    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 1/2] migration: Fix postcopy latency distribution formatting computation

[Philippe Mathieu-Daudé]

On 15/7/25 14:45, Fabiano Rosas wrote:
> 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>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   migration/migration-hmp-cmds.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

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

Re: [PATCH 2/2] cutils: Add time_us_to_str

[Philippe Mathieu-Daudé]

On 15/7/25 14:45, Fabiano Rosas wrote:
> 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>
> 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..7621726621 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 val);

s/val/us/

>   
>   /**
>    * freq_to_str:
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index bb954881d7..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..023793211a 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)
> +{
> +    const char *units[] = {"us", "ms", "sec"};

(static)

> +    int index = 0;
> +
> +    while (us > 1000 && index + 1 < ARRAY_SIZE(units)) {
> +        us /= 1000;
> +        index++;
> +    }
> +
> +    return g_strdup_printf("%"PRIu64" %s", us, units[index]);
> +}

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

Thanks!

Re: [PATCH 1/2] migration: Fix postcopy latency distribution formatting computation

[Prasad Pandit]

On Tue, 15 Jul 2025 at 18:49, Fabiano Rosas <farosas@suse.de> wrote:
> @@ -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]);

* This loop is rather confusing.

* Is the while loop converting microseconds (us) to seconds with:  us
/= 1000 ?  ie. index shall mostly be 2 = "sec", except for the range =
1000000 - 1000999,  when us / 1000 => 1000 would break the while loop
and it'd return string "1000 ms".
===
#define MS  (1000)
#define US  (MS * 1000)
#define NS  (US * 1000)

    if (n >= NS)
        n /= NS;
    else if (n >= US)
        n /= US;
    else if (n >= MS)
        n /= MS;

    return g_strdup_printf("%"PRIu64" sec", n);
===

* Does the above simplification look right? It shall always return
seconds as:  "<n> sec"


Thank you.
---
  - Prasad

Re: [PATCH 1/2] migration: Fix postcopy latency distribution formatting computation

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> On Tue, 15 Jul 2025 at 18:49, Fabiano Rosas <farosas@suse.de> wrote:
>> @@ -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]);
>
> * This loop is rather confusing.
>
> * Is the while loop converting microseconds (us) to seconds with:  us
> /= 1000 ?  ie. index shall mostly be 2 = "sec", except for the range =
> 1000000 - 1000999,  when us / 1000 => 1000 would break the while loop
> and it'd return string "1000 ms".

Good catch. The condition should be >=.

> ===
> #define MS  (1000)
> #define US  (MS * 1000)
> #define NS  (US * 1000)
>
>     if (n >= NS)
>         n /= NS;
>     else if (n >= US)
>         n /= US;
>     else if (n >= MS)
>         n /= MS;
>
>     return g_strdup_printf("%"PRIu64" sec", n);
> ===
>
> * Does the above simplification look right? It shall always return
> seconds as:  "<n> sec"
>

But then that's "0 sec" for 1000000 us.

>
> Thank you.
> ---
>   - Prasad

Re: [PATCH 1/2] migration: Fix postcopy latency distribution formatting computation

[Prasad Pandit]

On Wed, 16 Jul 2025 at 19:06, Fabiano Rosas <farosas@suse.de> wrote:
> The condition should be >=.

* I'm thinking of doing away with the loop and the string array. The
array has 3 values of which only one gets used due to conversion to
seconds.

> But then that's "0 sec" for 1000000 us.

#define US  (MS * 1000) => 1000000

When us = 1000000,  us / US should return "1 sec", no?

Thank you.
---
  - Prasad

Re: [PATCH 1/2] migration: Fix postcopy latency distribution formatting computation

[Fabiano Rosas]

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 16 Jul 2025 at 19:06, Fabiano Rosas <farosas@suse.de> wrote:
>> The condition should be >=.
>
> * I'm thinking of doing away with the loop and the string array. The
> array has 3 values of which only one gets used due to conversion to
> seconds.

The point is not to convert to seconds, but to fit the number of
microseconds in a range. Take a look at the output of 'info migrate -a'
with postcopy-blocktime capability enabled.

>
>> But then that's "0 sec" for 1000000 us.
>
> #define US  (MS * 1000) => 1000000
>
> When us = 1000000,  us / US should return "1 sec", no?
>

Sorry, I meant when the number of microseconds is smaller than a second.

> Thank you.
> ---
>   - Prasad

Re: [PATCH 1/2] migration: Fix postcopy latency distribution formatting computation

[Daniel P. Berrangé]

On Tue, Jul 15, 2025 at 09:45:51AM -0300, Fabiano Rosas wrote:
> 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>
> 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]);

It occurrs to me that this is the same basic algorithmic problem as
converting storage sizes from bytes to KB/MB/GB/etc.

We have size_to_str() which  does this conversion without needing any
loop at all.

Then there is freq_to_str() which has similar purpose but still uses
a loop, instead of the shortcut size_to_str has.

IMHO we should have a 'scaled_int_to_str' method that is common to
any place we need such scaled integer string conversions, and then
wrappers like  "size_to_str", "freq_to_str" and "duration_to_str"
that pass in the required list of unit strings, and the divisor
and requested decimal precision, etc.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|