[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] util: mdev: Use a local variable instead of an invalid pointer reference



On Wed, May 03, 2017 at 05:35:47PM +0200, Pavel Hrdina wrote:
> On Fri, Apr 28, 2017 at 09:46:32AM +0200, Erik Skultety wrote:
> > virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears
> > the element to be added, thus the pointer must not be used afterwards,
> > but because the pointer here is passed by value, what gets cleared is
> > a copy of the original pointer that got created on the stack
> > automatically when calling the function. The fact that it works now is
> > just a coincidence and a bug in virMediatedDeviceListAdd that needs to
> > be fixed in a separate commit. Therefore, use a local variable to hold
> > data, rather than accessing the pointer later.
> 
> I don't thing that this patch is required.  What happens here is that
> in this function we get a pointer to an existing memory that stores
> virMediatedDevice, if the mdev is not used we mark it as used and store
> that pointer to *dst* list. That's what virMediatedDeviceListAdd does,
> it only stores the pointer to an allocated into the list so it doesn't
> clear the real data, it only operates with pointers.  This means that
> the memory addressed by local *mdev* variable still points to a
> valid memory and everything is OK, there is no issue at all apart
> from a fact, that setting the pointer in virMediatedDeviceListAdd
> is pointless.
> 
> Pavel

After reading the second patch, this one will be required, however
I would change the commit message because there is no issue right
now, this change is only necessary for the following patch.

Pavel

> 
> > Signed-off-by: Erik Skultety <eskultet redhat com>
> > ---
> >  src/util/virmdev.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > index 2a1ade739..c1499d238 100644
> > --- a/src/util/virmdev.c
> > +++ b/src/util/virmdev.c
> > @@ -447,20 +447,21 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst,
> >      virObjectLock(dst);
> >      for (i = 0; i < count; i++) {
> >          virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i);
> > +        const char *mdev_path = mdev->path;
> >  
> >          if (virMediatedDeviceIsUsed(mdev, dst) ||
> >              virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0)
> >              goto cleanup;
> >  
> >          /* Copy mdev references to the driver list:
> > -         * - caller is responsible for NOT freeing devices in @list on success
> > +         * - caller is responsible for NOT freeing devices in @src on success
> >           * - we're responsible for performing a rollback on failure
> >           */
> >          if (virMediatedDeviceListAdd(dst, mdev) < 0)
> >              goto rollback;
> >  
> >          VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'",
> > -                  mdev->path, domname);
> > +                  mdev_path, domname);
> >      }
> >  
> >      ret = 0;
> > -- 
> > 2.12.2
> > 
> > --
> > libvir-list mailing list
> > libvir-list redhat com
> > https://www.redhat.com/mailman/listinfo/libvir-list



> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]