| Message ID | 150100553345.27487.10049014405920351882.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 25, 2017 at 07:58:53PM +0200, Greg Kurz wrote: > Passing a stack allocated buffer of arbitrary length to snprintf() > without checking the return value can cause the resultant strings > to be silently truncated. > > Signed-off-by: Greg Kurz <groug@kaod.org> Applied to ppc-for-2.11. > --- > hw/ppc/spapr_drc.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 15bae5c216a9..e4e8383ec7b5 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -488,7 +488,7 @@ static void realize(DeviceState *d, Error **errp) > { > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > Object *root_container; > - char link_name[256]; > + gchar *link_name; > gchar *child_name; > Error *err = NULL; > > @@ -501,11 +501,12 @@ static void realize(DeviceState *d, Error **errp) > * existing in the composition tree > */ > root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); > - snprintf(link_name, sizeof(link_name), "%x", spapr_drc_index(drc)); > + link_name = g_strdup_printf("%x", spapr_drc_index(drc)); > child_name = object_get_canonical_path_component(OBJECT(drc)); > trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name); > object_property_add_alias(root_container, link_name, > drc->owner, child_name, &err); > + g_free(link_name); > if (err) { > error_report_err(err); > object_unref(OBJECT(drc)); > @@ -521,13 +522,14 @@ static void unrealize(DeviceState *d, Error **errp) > { > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > Object *root_container; > - char name[256]; > + gchar *name; > Error *err = NULL; > > trace_spapr_drc_unrealize(spapr_drc_index(drc)); > root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); > - snprintf(name, sizeof(name), "%x", spapr_drc_index(drc)); > + name = g_strdup_printf("%x", spapr_drc_index(drc)); > object_property_del(root_container, name, &err); > + g_free(name); > if (err) { > error_report_err(err); > object_unref(OBJECT(drc)); > @@ -729,10 +731,11 @@ static const TypeInfo spapr_drc_lmb_info = { > sPAPRDRConnector *spapr_drc_by_index(uint32_t index) > { > Object *obj; > - char name[256]; > + gchar *name; > > - snprintf(name, sizeof(name), "%s/%x", DRC_CONTAINER_PATH, index); > + name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH, index); > obj = object_resolve_path(name, NULL); > + g_free(name); > > return !obj ? NULL : SPAPR_DR_CONNECTOR(obj); > } >
Hi David, On 07/26/2017 12:58 AM, David Gibson wrote: > On Tue, Jul 25, 2017 at 07:58:53PM +0200, Greg Kurz wrote: >> Passing a stack allocated buffer of arbitrary length to snprintf() >> without checking the return value can cause the resultant strings >> to be silently truncated. >> >> Signed-off-by: Greg Kurz <groug@kaod.org> > > Applied to ppc-for-2.11. Isn't it 2.10 material? Regards, Phil. > >> --- >> hw/ppc/spapr_drc.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c >> index 15bae5c216a9..e4e8383ec7b5 100644 >> --- a/hw/ppc/spapr_drc.c >> +++ b/hw/ppc/spapr_drc.c >> @@ -488,7 +488,7 @@ static void realize(DeviceState *d, Error **errp) >> { >> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); >> Object *root_container; >> - char link_name[256]; >> + gchar *link_name; >> gchar *child_name; >> Error *err = NULL; >> >> @@ -501,11 +501,12 @@ static void realize(DeviceState *d, Error **errp) >> * existing in the composition tree >> */ >> root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); >> - snprintf(link_name, sizeof(link_name), "%x", spapr_drc_index(drc)); >> + link_name = g_strdup_printf("%x", spapr_drc_index(drc)); >> child_name = object_get_canonical_path_component(OBJECT(drc)); >> trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name); >> object_property_add_alias(root_container, link_name, >> drc->owner, child_name, &err); >> + g_free(link_name); >> if (err) { >> error_report_err(err); >> object_unref(OBJECT(drc)); >> @@ -521,13 +522,14 @@ static void unrealize(DeviceState *d, Error **errp) >> { >> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); >> Object *root_container; >> - char name[256]; >> + gchar *name; >> Error *err = NULL; >> >> trace_spapr_drc_unrealize(spapr_drc_index(drc)); >> root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); >> - snprintf(name, sizeof(name), "%x", spapr_drc_index(drc)); >> + name = g_strdup_printf("%x", spapr_drc_index(drc)); >> object_property_del(root_container, name, &err); >> + g_free(name); >> if (err) { >> error_report_err(err); >> object_unref(OBJECT(drc)); >> @@ -729,10 +731,11 @@ static const TypeInfo spapr_drc_lmb_info = { >> sPAPRDRConnector *spapr_drc_by_index(uint32_t index) >> { >> Object *obj; >> - char name[256]; >> + gchar *name; >> >> - snprintf(name, sizeof(name), "%s/%x", DRC_CONTAINER_PATH, index); >> + name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH, index); >> obj = object_resolve_path(name, NULL); >> + g_free(name); >> >> return !obj ? NULL : SPAPR_DR_CONNECTOR(obj); >> } >> >
On Mon, 31 Jul 2017 07:11:45 -0300 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi David, > > On 07/26/2017 12:58 AM, David Gibson wrote: > > On Tue, Jul 25, 2017 at 07:58:53PM +0200, Greg Kurz wrote: > >> Passing a stack allocated buffer of arbitrary length to snprintf() > >> without checking the return value can cause the resultant strings > >> to be silently truncated. > >> > >> Signed-off-by: Greg Kurz <groug@kaod.org> > > > > Applied to ppc-for-2.11. > > Isn't it 2.10 material? > Hi Philippe, Well... this patch doesn't fix any bug actually since the stack buffers are large enough. It is more a question of coding style. Something like below would have been more appropriate I guess: "Building strings with g_strdup_printf() is a QEMU common practice." No big deal. Cheers, -- Greg > Regards, > > Phil. > > > > >> --- > >> hw/ppc/spapr_drc.c | 15 +++++++++------ > >> 1 file changed, 9 insertions(+), 6 deletions(-) > >> > >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > >> index 15bae5c216a9..e4e8383ec7b5 100644 > >> --- a/hw/ppc/spapr_drc.c > >> +++ b/hw/ppc/spapr_drc.c > >> @@ -488,7 +488,7 @@ static void realize(DeviceState *d, Error **errp) > >> { > >> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > >> Object *root_container; > >> - char link_name[256]; > >> + gchar *link_name; > >> gchar *child_name; > >> Error *err = NULL; > >> > >> @@ -501,11 +501,12 @@ static void realize(DeviceState *d, Error **errp) > >> * existing in the composition tree > >> */ > >> root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); > >> - snprintf(link_name, sizeof(link_name), "%x", spapr_drc_index(drc)); > >> + link_name = g_strdup_printf("%x", spapr_drc_index(drc)); > >> child_name = object_get_canonical_path_component(OBJECT(drc)); > >> trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name); > >> object_property_add_alias(root_container, link_name, > >> drc->owner, child_name, &err); > >> + g_free(link_name); > >> if (err) { > >> error_report_err(err); > >> object_unref(OBJECT(drc)); > >> @@ -521,13 +522,14 @@ static void unrealize(DeviceState *d, Error **errp) > >> { > >> sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > >> Object *root_container; > >> - char name[256]; > >> + gchar *name; > >> Error *err = NULL; > >> > >> trace_spapr_drc_unrealize(spapr_drc_index(drc)); > >> root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); > >> - snprintf(name, sizeof(name), "%x", spapr_drc_index(drc)); > >> + name = g_strdup_printf("%x", spapr_drc_index(drc)); > >> object_property_del(root_container, name, &err); > >> + g_free(name); > >> if (err) { > >> error_report_err(err); > >> object_unref(OBJECT(drc)); > >> @@ -729,10 +731,11 @@ static const TypeInfo spapr_drc_lmb_info = { > >> sPAPRDRConnector *spapr_drc_by_index(uint32_t index) > >> { > >> Object *obj; > >> - char name[256]; > >> + gchar *name; > >> > >> - snprintf(name, sizeof(name), "%s/%x", DRC_CONTAINER_PATH, index); > >> + name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH, index); > >> obj = object_resolve_path(name, NULL); > >> + g_free(name); > >> > >> return !obj ? NULL : SPAPR_DR_CONNECTOR(obj); > >> } > >> > >
On Mon, Jul 31, 2017 at 12:34:41PM +0200, Greg Kurz wrote: > On Mon, 31 Jul 2017 07:11:45 -0300 > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > Hi David, > > > > On 07/26/2017 12:58 AM, David Gibson wrote: > > > On Tue, Jul 25, 2017 at 07:58:53PM +0200, Greg Kurz wrote: > > >> Passing a stack allocated buffer of arbitrary length to snprintf() > > >> without checking the return value can cause the resultant strings > > >> to be silently truncated. > > >> > > >> Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > Applied to ppc-for-2.11. > > > > Isn't it 2.10 material? > > > > Hi Philippe, > > Well... this patch doesn't fix any bug actually since the stack buffers > are large enough. It is more a question of coding style. > > Something like below would have been more appropriate I guess: > > "Building strings with g_strdup_printf() is a QEMU common practice." > > No big deal. Exactly. It's not a bugfix, so it doesn't go into 2.10 - we've passed the hard freeze.
On 07/31/2017 09:53 AM, David Gibson wrote: > On Mon, Jul 31, 2017 at 12:34:41PM +0200, Greg Kurz wrote: >> On Mon, 31 Jul 2017 07:11:45 -0300 >> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >>> Hi David, >>> >>> On 07/26/2017 12:58 AM, David Gibson wrote: >>>> On Tue, Jul 25, 2017 at 07:58:53PM +0200, Greg Kurz wrote: >>>>> Passing a stack allocated buffer of arbitrary length to snprintf() >>>>> without checking the return value can cause the resultant strings >>>>> to be silently truncated. >>>>> >>>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>> >>>> Applied to ppc-for-2.11. >>> >>> Isn't it 2.10 material? >>> >> >> Hi Philippe, >> >> Well... this patch doesn't fix any bug actually since the stack buffers >> are large enough. It is more a question of coding style. >> >> Something like below would have been more appropriate I guess: >> >> "Building strings with g_strdup_printf() is a QEMU common practice." >> >> No big deal. > > Exactly. It's not a bugfix, so it doesn't go into 2.10 - we've passed > the hard freeze. Fair enough :)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 15bae5c216a9..e4e8383ec7b5 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -488,7 +488,7 @@ static void realize(DeviceState *d, Error **errp) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); Object *root_container; - char link_name[256]; + gchar *link_name; gchar *child_name; Error *err = NULL; @@ -501,11 +501,12 @@ static void realize(DeviceState *d, Error **errp) * existing in the composition tree */ root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); - snprintf(link_name, sizeof(link_name), "%x", spapr_drc_index(drc)); + link_name = g_strdup_printf("%x", spapr_drc_index(drc)); child_name = object_get_canonical_path_component(OBJECT(drc)); trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name); object_property_add_alias(root_container, link_name, drc->owner, child_name, &err); + g_free(link_name); if (err) { error_report_err(err); object_unref(OBJECT(drc)); @@ -521,13 +522,14 @@ static void unrealize(DeviceState *d, Error **errp) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); Object *root_container; - char name[256]; + gchar *name; Error *err = NULL; trace_spapr_drc_unrealize(spapr_drc_index(drc)); root_container = container_get(object_get_root(), DRC_CONTAINER_PATH); - snprintf(name, sizeof(name), "%x", spapr_drc_index(drc)); + name = g_strdup_printf("%x", spapr_drc_index(drc)); object_property_del(root_container, name, &err); + g_free(name); if (err) { error_report_err(err); object_unref(OBJECT(drc)); @@ -729,10 +731,11 @@ static const TypeInfo spapr_drc_lmb_info = { sPAPRDRConnector *spapr_drc_by_index(uint32_t index) { Object *obj; - char name[256]; + gchar *name; - snprintf(name, sizeof(name), "%s/%x", DRC_CONTAINER_PATH, index); + name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH, index); obj = object_resolve_path(name, NULL); + g_free(name); return !obj ? NULL : SPAPR_DR_CONNECTOR(obj); }
Passing a stack allocated buffer of arbitrary length to snprintf() without checking the return value can cause the resultant strings to be silently truncated. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr_drc.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)