Re: [libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak

[Erik Skultety]

> >> VIR_FREE() would have to be done at the top of the function; otherwise,
> >> how does the caller distinguish which error occurred when -1 gets
> >> returned and whether it should VIR_FREE itself?
> >>
> > 
> > Well, I have to admin that this^^ is a fair argument because there are 3
> > different spots where the function can fail, not that the caller could not
> > check result for NULL but the fact that a function touched caller's argument
> > and then failed would be just weird. So, yeah, good point.
> > 
> I actually thought this was the "more compelling" reason, but seeing as
> there's no other feedback - I'll make the simple patch for having the
> VIR_FREE() in virReadFCHost, adjust the comments, and move on.
> 
> John
> 

Hi John, I think I admitted that you had a very good point (the one on top) so
I thought you would actually push your original version, I'm sorry if I wasn't
clear enough with my statement.

Erik


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak

[John Ferlan]

On 10/12/2016 10:49 AM, Erik Skultety wrote:
> On Wed, Oct 12, 2016 at 09:20:29AM -0400, John Ferlan wrote:
>>
>>
>> On 10/12/2016 06:40 AM, Erik Skultety wrote:
>>> On Tue, Oct 11, 2016 at 05:25:49PM -0400, John Ferlan wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1357416

 Rather than return a 0 or -1 and the *result string, return just the result
 string to the caller.  Alter all the callers to handle the different 
 return.

 As a side effect or result of this, it's much clearer that we cannot just
 assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn
 fields - rather we should fetch a temporary string, then as long as our
 fetch was good, VIR_FREE what may have been there, and STEAL what we just 
 got.
 This fixes a memory leak in the virNodeDeviceCreateXML code path through
 find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually
 call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found
 in the device object capabilities.

 Signed-off-by: John Ferlan 
 ---

  I suppose I could have made two patches out of this, but it felt like
  overkill once I realized what the issue was. OK a third one line patch
  could have been added to change the virGetFCHostNameByWWN comment as well,
  but that'd really be overkill.

  src/node_device/node_device_linux_sysfs.c | 55 
 ---
  src/util/virutil.c| 34 ---
  src/util/virutil.h|  9 +++--
  tests/fchosttest.c| 30 ++---
  4 files changed, 49 insertions(+), 79 deletions(-)

 diff --git a/src/node_device/node_device_linux_sysfs.c 
 b/src/node_device/node_device_linux_sysfs.c
 index 549d32c..be99c41 100644
 --- a/src/node_device/node_device_linux_sysfs.c
 +++ b/src/node_device/node_device_linux_sysfs.c
 @@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs");
  int
  nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
  {
 -char *max_vports = NULL;
 -char *vports = NULL;
 +char *tmp = NULL;
  int ret = -1;
  
  if (virReadSCSIUniqueId(NULL, d->scsi_host.host,
 @@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
  if (virIsCapableFCHost(NULL, d->scsi_host.host)) {
  d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
  
 -if (virReadFCHost(NULL,
 -  d->scsi_host.host,
 -  "port_name",
 -  >scsi_host.wwpn) < 0) {
 +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) 
 {
  VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host);
  goto cleanup;
  }
 +VIR_FREE(d->scsi_host.wwpn);
 +VIR_STEAL_PTR(d->scsi_host.wwpn, tmp);
  
 -if (virReadFCHost(NULL,
 -  d->scsi_host.host,
 -  "node_name",
 -  >scsi_host.wwnn) < 0) {
 +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) 
 {
  VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host);
  goto cleanup;
  }
 +VIR_FREE(d->scsi_host.wwnn);
 +VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
  
 -if (virReadFCHost(NULL,
 -  d->scsi_host.host,
 -  "fabric_name",
 -  >scsi_host.fabric_wwn) < 0) {
 +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, 
 "fabric_name"))) {
  VIR_WARN("Failed to read fabric WWN for host%d",
   d->scsi_host.host);
  goto cleanup;
  }
 +VIR_FREE(d->scsi_host.fabric_wwn);
 +VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
  }
  
>>>
>>> So if I understand correctly, the problem is basically that we did not call
>>> VIR_FREE on the data that had previously been filled to the 
>>> virNodeDevCapData
>>> structure during replacing it with new data. So, we could just keep the
>>> signature of virReadFCHost as is and just add the VIR_FREE and VIR_STEAL_PTR
>>> to the function, thus there won't be any need to use VIR_FREE+STEAL 
>>> repeatedly
>>> in here.
>>>
>>
>> True - we're overwriting the >scsi_host.{wwnn|wwpn|fabric_wwn} fields
>> and that's the primary issue (e.g. mem leak).
>>
>> While I understand your point, I'm not sure adding a VIR_FREE(*result)
>> to virReadFCHost just prior to the "if VIR_STRDUP(*result, p) < 0)" is a
>> proper solution since virReadFCHost does not "own" that memory. It
>> doesn't state that if *result has 

Re: [libvirt] [PATCH] util: Alter return value of virReadFCHost and fix mem leak

[Erik Skultety]

On Wed, Oct 12, 2016 at 09:20:29AM -0400, John Ferlan wrote:
> 
> 
> On 10/12/2016 06:40 AM, Erik Skultety wrote:
> > On Tue, Oct 11, 2016 at 05:25:49PM -0400, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1357416
> >>
> >> Rather than return a 0 or -1 and the *result string, return just the result
> >> string to the caller.  Alter all the callers to handle the different 
> >> return.
> >>
> >> As a side effect or result of this, it's much clearer that we cannot just
> >> assign the returned string into the scsi_host wwnn, wwpn, and fabric_wwn
> >> fields - rather we should fetch a temporary string, then as long as our
> >> fetch was good, VIR_FREE what may have been there, and STEAL what we just 
> >> got.
> >> This fixes a memory leak in the virNodeDeviceCreateXML code path through
> >> find_new_device and nodeDeviceLookupSCSIHostByWWN which will continually
> >> call nodeDeviceSysfsGetSCSIHostCaps until the expected wwnn/wwpn is found
> >> in the device object capabilities.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>
> >>  I suppose I could have made two patches out of this, but it felt like
> >>  overkill once I realized what the issue was. OK a third one line patch
> >>  could have been added to change the virGetFCHostNameByWWN comment as well,
> >>  but that'd really be overkill.
> >>
> >>  src/node_device/node_device_linux_sysfs.c | 55 
> >> ---
> >>  src/util/virutil.c| 34 ---
> >>  src/util/virutil.h|  9 +++--
> >>  tests/fchosttest.c| 30 ++---
> >>  4 files changed, 49 insertions(+), 79 deletions(-)
> >>
> >> diff --git a/src/node_device/node_device_linux_sysfs.c 
> >> b/src/node_device/node_device_linux_sysfs.c
> >> index 549d32c..be99c41 100644
> >> --- a/src/node_device/node_device_linux_sysfs.c
> >> +++ b/src/node_device/node_device_linux_sysfs.c
> >> @@ -44,8 +44,7 @@ VIR_LOG_INIT("node_device.node_device_linux_sysfs");
> >>  int
> >>  nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
> >>  {
> >> -char *max_vports = NULL;
> >> -char *vports = NULL;
> >> +char *tmp = NULL;
> >>  int ret = -1;
> >>  
> >>  if (virReadSCSIUniqueId(NULL, d->scsi_host.host,
> >> @@ -59,64 +58,53 @@ nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapDataPtr d)
> >>  if (virIsCapableFCHost(NULL, d->scsi_host.host)) {
> >>  d->scsi_host.flags |= VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST;
> >>  
> >> -if (virReadFCHost(NULL,
> >> -  d->scsi_host.host,
> >> -  "port_name",
> >> -  >scsi_host.wwpn) < 0) {
> >> +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "port_name"))) 
> >> {
> >>  VIR_WARN("Failed to read WWPN for host%d", d->scsi_host.host);
> >>  goto cleanup;
> >>  }
> >> +VIR_FREE(d->scsi_host.wwpn);
> >> +VIR_STEAL_PTR(d->scsi_host.wwpn, tmp);
> >>  
> >> -if (virReadFCHost(NULL,
> >> -  d->scsi_host.host,
> >> -  "node_name",
> >> -  >scsi_host.wwnn) < 0) {
> >> +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, "node_name"))) 
> >> {
> >>  VIR_WARN("Failed to read WWNN for host%d", d->scsi_host.host);
> >>  goto cleanup;
> >>  }
> >> +VIR_FREE(d->scsi_host.wwnn);
> >> +VIR_STEAL_PTR(d->scsi_host.wwnn, tmp);
> >>  
> >> -if (virReadFCHost(NULL,
> >> -  d->scsi_host.host,
> >> -  "fabric_name",
> >> -  >scsi_host.fabric_wwn) < 0) {
> >> +if (!(tmp = virReadFCHost(NULL, d->scsi_host.host, 
> >> "fabric_name"))) {
> >>  VIR_WARN("Failed to read fabric WWN for host%d",
> >>   d->scsi_host.host);
> >>  goto cleanup;
> >>  }
> >> +VIR_FREE(d->scsi_host.fabric_wwn);
> >> +VIR_STEAL_PTR(d->scsi_host.fabric_wwn, tmp);
> >>  }
> >>  
> > 
> > So if I understand correctly, the problem is basically that we did not call
> > VIR_FREE on the data that had previously been filled to the 
> > virNodeDevCapData
> > structure during replacing it with new data. So, we could just keep the
> > signature of virReadFCHost as is and just add the VIR_FREE and VIR_STEAL_PTR
> > to the function, thus there won't be any need to use VIR_FREE+STEAL 
> > repeatedly
> > in here.
> > 
> 
> True - we're overwriting the >scsi_host.{wwnn|wwpn|fabric_wwn} fields
> and that's the primary issue (e.g. mem leak).
> 
> While I understand your point, I'm not sure adding a VIR_FREE(*result)
> to virReadFCHost just prior to the "if VIR_STRDUP(*result, p) < 0)" is a
> proper solution since virReadFCHost does not "own" that memory. It
> doesn't state that if *result has something in it, the code will perform
> a VIR_FREE