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 INT16_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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qom/object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

changelog:
v1: initial version
v2: change INT_MAX to INT16_MAX in code
v3: make the same change in commit log. Sorry for missing it.

diff --git a/qom/object.c b/qom/object.c
index 387efb25eb..9962874598 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1166,11 +1166,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 < INT16_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,
@@ -1181,6 +1181,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

Request to queue this patch for the next pull.

On Mon, Sep 21, 2020 at 15:03 Ani Sinha <ani@anisinha.ca> 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 INT16_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>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qom/object.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> changelog:
> v1: initial version
> v2: change INT_MAX to INT16_MAX in code
> v3: make the same change in commit log. Sorry for missing it.
>
> diff --git a/qom/object.c b/qom/object.c
> index 387efb25eb..9962874598 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1166,11 +1166,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 < INT16_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,
> @@ -1181,6 +1181,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
>
>

Ping ...

On Mon, Oct 12, 2020 at 8:08 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> Request to queue this patch for the next pull.
>
> On Mon, Sep 21, 2020 at 15:03 Ani Sinha <ani@anisinha.ca> 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 INT16_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>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>  qom/object.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> changelog:
>> v1: initial version
>> v2: change INT_MAX to INT16_MAX in code
>> v3: make the same change in commit log. Sorry for missing it.
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 387efb25eb..9962874598 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1166,11 +1166,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 < INT16_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,
>> @@ -1181,6 +1181,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
>>

On Mon, Sep 21, 2020 at 03:03:25PM +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 INT16_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>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Queued on machine-next, thanks!  My apologies for the delay.

-- 
Eduardo

On Thu, Oct 15, 2020 at 10:22 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Mon, Sep 21, 2020 at 03:03:25PM +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 INT16_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>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> Queued on machine-next, thanks!  My apologies for the delay.

Any idea when this will be pulled?

Ping ...

On Sat, Oct 31, 2020 at 9:51 PM Ani Sinha <ani@anisinha.ca> wrote:

> On Thu, Oct 15, 2020 at 10:22 PM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> >
> > On Mon, Sep 21, 2020 at 03:03:25PM +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 INT16_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>
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> > Queued on machine-next, thanks!  My apologies for the delay.
>
> Any idea when this will be pulled?
>

On Sat, Oct 31, 2020 at 09:51:38PM +0530, Ani Sinha wrote:
> On Thu, Oct 15, 2020 at 10:22 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Mon, Sep 21, 2020 at 03:03:25PM +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 INT16_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>
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> > Queued on machine-next, thanks!  My apologies for the delay.
> 
> Any idea when this will be pulled?

I was planning to send a pull request before soft freeze,
(October 27) but this was the only patch in the queue at that
moment, so I decided to wait.

As we're beyond freeze now, the pull request will be sent
immediately after 5.2.0 is released.

-- 
Eduardo

On Wed, Nov 4, 2020 at 21:29 Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sat, Oct 31, 2020 at 09:51:38PM +0530, Ani Sinha wrote:
> > On Thu, Oct 15, 2020 at 10:22 PM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> > >
> > > On Mon, Sep 21, 2020 at 03:03:25PM +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 INT16_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>
> > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > >
> > > Queued on machine-next, thanks!  My apologies for the delay.
> >
> > Any idea when this will be pulled?
>
> I was planning to send a pull request before soft freeze,
> (October 27) but this was the only patch in the queue at that
> moment, so I decided to wait.
>
> As we're beyond freeze now, the pull request will be sent
> immediately after 5.2.0 is released.


Gentle reminder since 5.2.0 has now been released.


>
>