| Message ID | 20200921093325.25617-1-ani@anisinha.ca |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] qom: code hardening - have bound checking while looping with integer value | expand |
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 > > <div dir="auto">Request to queue this patch for the next pull.</div><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 21, 2020 at 15:03 Ani Sinha <<a href="mailto:ani@anisinha.ca">ani@anisinha.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204,204,204)">Object property insertion code iterates over an integer to get an unused<br> index that can be used as an unique name for an object property. This loop<br> increments the integer value indefinitely. Although very unlikely, this can<br> still cause an integer overflow.<br> In this change, we fix the above code by checking against INT16_MAX and making<br> sure that the interger index does not overflow beyond that value. If no<br> available index is found, the code would cause an assertion failure. This<br> assertion failure is necessary because the callers of the function do not check<br> the return value for NULL.<br> <br> Signed-off-by: Ani Sinha <<a href="mailto:ani@anisinha.ca" target="_blank">ani@anisinha.ca</a>><br> Reviewed-by: Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>><br> ---<br> qom/object.c | 5 +++--<br> 1 file changed, 3 insertions(+), 2 deletions(-)<br> <br> changelog:<br> v1: initial version<br> v2: change INT_MAX to INT16_MAX in code<br> v3: make the same change in commit log. Sorry for missing it.<br> <br> diff --git a/qom/object.c b/qom/object.c<br> index 387efb25eb..9962874598 100644<br> --- a/qom/object.c<br> +++ b/qom/object.c<br> @@ -1166,11 +1166,11 @@ object_property_try_add(Object *obj, const char *name, const char *type,<br> <br> if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {<br> int i;<br> - ObjectProperty *ret;<br> + ObjectProperty *ret = NULL;<br> char *name_no_array = g_strdup(name);<br> <br> name_no_array[name_len - 3] = '\0';<br> - for (i = 0; ; ++i) {<br> + for (i = 0; i < INT16_MAX; ++i) {<br> char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);<br> <br> ret = object_property_try_add(obj, full_name, type, get, set,<br> @@ -1181,6 +1181,7 @@ object_property_try_add(Object *obj, const char *name, const char *type,<br> }<br> }<br> g_free(name_no_array);<br> + assert(ret);<br> return ret;<br> }<br> <br> -- <br> 2.17.1<br> <br> </blockquote></div></div>
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? > <div dir="auto">Ping ...</div><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Oct 31, 2020 at 9:51 PM Ani Sinha <<a href="mailto:ani@anisinha.ca">ani@anisinha.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rgb(204,204,204)">On Thu, Oct 15, 2020 at 10:22 PM Eduardo Habkost <<a href="mailto:ehabkost@redhat.com" target="_blank">ehabkost@redhat.com</a>> wrote:<br> ><br> > On Mon, Sep 21, 2020 at 03:03:25PM +0530, Ani Sinha wrote:<br> > > Object property insertion code iterates over an integer to get an unused<br> > > index that can be used as an unique name for an object property. This loop<br> > > increments the integer value indefinitely. Although very unlikely, this can<br> > > still cause an integer overflow.<br> > > In this change, we fix the above code by checking against INT16_MAX and making<br> > > sure that the interger index does not overflow beyond that value. If no<br> > > available index is found, the code would cause an assertion failure. This<br> > > assertion failure is necessary because the callers of the function do not check<br> > > the return value for NULL.<br> > ><br> > > Signed-off-by: Ani Sinha <<a href="mailto:ani@anisinha.ca" target="_blank">ani@anisinha.ca</a>><br> > > Reviewed-by: Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>><br> ><br> > Queued on machine-next, thanks! My apologies for the delay.<br> <br> Any idea when this will be pulled?<br> </blockquote></div></div>
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. > > <div><br></div><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 4, 2020 at 21:29 Eduardo Habkost <<a href="mailto:ehabkost@redhat.com">ehabkost@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, Oct 31, 2020 at 09:51:38PM +0530, Ani Sinha wrote:<br> > On Thu, Oct 15, 2020 at 10:22 PM Eduardo Habkost <<a href="mailto:ehabkost@redhat.com" target="_blank">ehabkost@redhat.com</a>> wrote:<br> > ><br> > > On Mon, Sep 21, 2020 at 03:03:25PM +0530, Ani Sinha wrote:<br> > > > Object property insertion code iterates over an integer to get an unused<br> > > > index that can be used as an unique name for an object property. This loop<br> > > > increments the integer value indefinitely. Although very unlikely, this can<br> > > > still cause an integer overflow.<br> > > > In this change, we fix the above code by checking against INT16_MAX and making<br> > > > sure that the interger index does not overflow beyond that value. If no<br> > > > available index is found, the code would cause an assertion failure. This<br> > > > assertion failure is necessary because the callers of the function do not check<br> > > > the return value for NULL.<br> > > ><br> > > > Signed-off-by: Ani Sinha <<a href="mailto:ani@anisinha.ca" target="_blank">ani@anisinha.ca</a>><br> > > > Reviewed-by: Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>><br> > ><br> > > Queued on machine-next, thanks! My apologies for the delay.<br> > <br> > Any idea when this will be pulled?<br> <br> I was planning to send a pull request before soft freeze,<br> (October 27) but this was the only patch in the queue at that<br> moment, so I decided to wait.<br> <br> As we're beyond freeze now, the pull request will be sent<br> immediately after 5.2.0 is released.</blockquote><div dir="auto"><br></div><div dir="auto">Gentle reminder since 5.2.0 has now been released.</div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex" dir="auto"><br> <br> </blockquote></div></div>
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; }