Re: [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan

[David Marchand <david.marchand@redhat.com>]

On Wed, Sep 29, 2021 at 10:57 PM Long Li <longli@microsoft.com> wrote:
>
> > Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
> >
> > Caught running ASAN.
> >
> > The device name is leaked on scan.
> > rte_device name field being a const, use the private vmbus struct to store the
> > device name and point at it.
> >
> > Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
> >  drivers/bus/vmbus/rte_bus_vmbus.h   | 1 +
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> > b/drivers/bus/vmbus/linux/vmbus_bus.c
> > index 3c924eee14..d8eb07d398 100644
> > --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> > +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> > @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
> >               return -1;
> >
> >       dev->device.bus = &rte_vmbus_bus.bus;
> > -     dev->device.name = strdup(name);
> > +     dev->device.name = dev->name = strdup(name);
>
>
> I suggest not to add a "name" in struct rte_vmbus_device. How about defining a local variable in this function, like:
> dev->device.name = dev_name = strdup(name);

What is your concern for storing the name?


rte_device name only points at some location where the name is stored.
In general this storage is in the bus object or (in some buses) the
devarg that resulted in the rte_device object creation.

If we won't store the name in the bus object, then we lose the ability
to release the name later.
This is probably fine as long as we never release rte_vmbus_device
objects which is the case atm.
But I don't understand why vmbus should be an exception when comparing
to other buses.


-- 
David Marchand