Re: [Qemu-devel] [for-2.11 PATCH 04/26] spapr_drc: use g_strdup_printf() instead of snprintf()

[Greg Kurz]

On Mon, 31 Jul 2017 07:11:45 -0300
Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>  
> > 
> > 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);
> >>   }
> >>  
> >

pgpH0y5lh84ZJ.pgp
Description: OpenPGP digital signature