[PATCH] qom: code hardening - have bound checking while looping with integer value

[PATCH] qom: code hardening - have bound checking while looping with integer value

[Ani Sinha]

Object property insertion code iterates over an integer to get an unused
index that can be used as an unique name for an object property. This loop
increments the integer value indefinitely. Although very unlikely, this can
still cause an integer overflow.
In this change, we fix the above code by checking against INT_MAX and making
sure that the interger index does not overflow beyond that value. If no
available index is found, the code would cause an assertion failure. This
assertion failure is necessary because the callers of the function do not check
the return value for NULL.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 qom/object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 00fdf89b3b..62414da67f 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1158,11 +1158,11 @@ object_property_try_add(Object *obj, const char *name, const char *type,
 
     if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
         int i;
-        ObjectProperty *ret;
+        ObjectProperty *ret = NULL;
         char *name_no_array = g_strdup(name);
 
         name_no_array[name_len - 3] = '\0';
-        for (i = 0; ; ++i) {
+        for (i = 0; i < INT_MAX; ++i) {
             char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
 
             ret = object_property_try_add(obj, full_name, type, get, set,
@@ -1173,6 +1173,7 @@ object_property_try_add(Object *obj, const char *name, const char *type,
             }
         }
         g_free(name_no_array);
+        assert(ret);
         return ret;
     }
 
-- 
2.17.1

Re: [PATCH] qom: code hardening - have bound checking while looping with integer value

[Daniel P. Berrangé]

On Sat, Sep 19, 2020 at 09:11:39PM +0530, Ani Sinha wrote:
> Object property insertion code iterates over an integer to get an unused
> index that can be used as an unique name for an object property. This loop
> increments the integer value indefinitely. Although very unlikely, this can
> still cause an integer overflow.
> In this change, we fix the above code by checking against INT_MAX and making
> sure that the interger index does not overflow beyond that value. If no
> available index is found, the code would cause an assertion failure. This
> assertion failure is necessary because the callers of the function do not check
> the return value for NULL.

If we're going to put a limit on the loop, then lets at least make it a
sensible real world limit. INT_MAX is insanely large, as a per-object
property count limit, we won't get within many orders of magnitude of
that.

Even INT8_MAX is probably large enough, but just in case someone has
a valid reason for 255 properties, INT16_MAX gives breathing space.
If someone needs 65535 properties on a single obj I'd call it a design
flaw.

> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  qom/object.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 00fdf89b3b..62414da67f 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1158,11 +1158,11 @@ object_property_try_add(Object *obj, const char *name, const char *type,
>  
>      if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
>          int i;
> -        ObjectProperty *ret;
> +        ObjectProperty *ret = NULL;
>          char *name_no_array = g_strdup(name);
>  
>          name_no_array[name_len - 3] = '\0';
> -        for (i = 0; ; ++i) {
> +        for (i = 0; i < INT_MAX; ++i) {
>              char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
>  
>              ret = object_property_try_add(obj, full_name, type, get, set,
> @@ -1173,6 +1173,7 @@ object_property_try_add(Object *obj, const char *name, const char *type,
>              }
>          }
>          g_free(name_no_array);
> +        assert(ret);
>          return ret;
>      }
>  
> -- 
> 2.17.1
> 

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 :|