Re: [RFC PATCH 1/2] node_device_conf: virNodeDeviceGetSCSITargetCaps: fix memory leak

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Wednesday in 2024, Marc Hartmayer wrote:
> Make sure the old value in `scsi_target->wwpn` is free'd before replacing it.
> While at it, simplify the code.

"While at it" usually means it should have been a separate commit,
especially if the simplification changes the behavior of the code.

> ==9104== 38 bytes in 2 blocks are definitely lost in loss record 1,943 of 3,250
> ==9104==    at 0x483B8C0: malloc (vg_replace_malloc.c:442)
> ==9104==    by 0x4DFB69B: g_malloc (gmem.c:130)
> ==9104==    by 0x4E1921D: g_strdup (gstrfuncs.c:363)
> ==9104==    by 0x495D60B: g_strdup_inline (gstrfuncs.h:321)
> ==9104==    by 0x495D60B: virFCReadRportValue (virfcp.c:62)
> ==9104==    by 0x4A5F5CB: virNodeDeviceGetSCSITargetCaps (node_device_conf.c:2914)
> ==9104==    by 0xBF62529: udevProcessSCSITarget (node_device_udev.c:657)
> ==9104==    by 0xBF62529: udevGetDeviceDetails (node_device_udev.c:1406)
> ==9104==    by 0xBF62529: udevAddOneDevice (node_device_udev.c:1563)
> ==9104==    by 0xBF639B5: udevProcessDeviceListEntry (node_device_udev.c:1637)
> ==9104==    by 0xBF639B5: udevEnumerateDevices (node_device_udev.c:1691)
> ==9104==    by 0xBF639B5: nodeStateInitializeEnumerate (node_device_udev.c:2009)
> ==9104==    by 0x49BDBFD: virThreadHelper (virthread.c:256)
> ==9104==    by 0x5242069: start_thread (in /usr/lib64/libc.so.6)
>
> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
> ---
> src/conf/node_device_conf.c | 29 +++++++++++------------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 48140b17baa5..c146a9f0c881 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2894,9 +2894,9 @@ int
> virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
>                                virNodeDevCapSCSITarget *scsi_target)
> {
> -    int ret = -1;
>     g_autofree char *dir = NULL;
>     g_autofree char *rport = NULL;
> +    g_autofree char *wwpn = NULL;
>
>     VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name);
>
> @@ -2906,28 +2906,21 @@ virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
>     rport = g_path_get_basename(dir);
>
>     if (!virFCIsCapableRport(rport))
> -        goto cleanup;
> +        return -1;
> +
> +    if (virFCReadRportValue(rport, "port_name",
> +                            &wwpn) < 0) {
> +        VIR_WARN("Failed to read port_name for '%s'", rport);
> +        return -1;
> +    }
>
>     VIR_FREE(scsi_target->rport);
>     scsi_target->rport = g_steal_pointer(&rport);
>
> -    if (virFCReadRportValue(scsi_target->rport, "port_name",
> -                            &scsi_target->wwpn) < 0) {
> -        VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport);
> -        goto cleanup;
> -    }
> -
> +    VIR_FREE(scsi_target->wwpn);
> +    scsi_target->wwpn = g_steal_pointer(&wwpn);
>     scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
> -    ret = 0;
> -
> - cleanup:
> -    if (ret < 0) {
> -        VIR_FREE(scsi_target->rport);
> -        VIR_FREE(scsi_target->wwpn);
> -        scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT;

Before, we cleared out the info if we were not able to refresh it.
Is there a possibility that would lead to stale information?

Jano

> -    }
> -
> -    return ret;
> +    return 0;
> }
>
>
> --
> 2.34.1
> _______________________________________________
> Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx