[libvirt] [PATCH v2 7/10] selinux: Resolve resource leak using the default disk label

[Peter Krempa]

On 01/18/13 15:34, John Ferlan wrote:
> Commit id a994ef2d1 changed the mechanism to store/update the default
> security label from using disk->seclabels[0] to allocating one on the
> fly. That change allocated the label, but never saved it.  This patch
> will save the label. The new virDomainDiskDefAddSecurityLabelDef() is
> a copy of the virDomainDefAddSecurityLabelDef().
> ---
>   src/conf/domain_conf.c          | 51 ++++++++++++++++++++++++++++++-----------
>   src/conf/domain_conf.h          |  3 +++
>   src/security/security_selinux.c |  6 ++---
>   3 files changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0b9ba13..7640af7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16041,26 +16041,51 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model)
>   {
>       virSecurityLabelDefPtr seclabel = NULL;
>
> -    if (VIR_ALLOC(seclabel) < 0) {
> -        virReportOOMError();
> -        return NULL;
> -    }
> +    if (VIR_ALLOC(seclabel) < 0)
> +        goto no_memory;
>
>       if (model) {
>           seclabel->model = strdup(model);
> -        if (seclabel->model == NULL) {
> -            virReportOOMError();
> -            virSecurityLabelDefFree(seclabel);
> -            return NULL;
> -        }
> +        if (seclabel->model == NULL)
> +            goto no_memory;
>       }
>
> -    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) {
> -        virReportOOMError();
> -        virSecurityLabelDefFree(seclabel);
> -        return NULL;
> +    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0)
> +        goto no_memory;
> +
> +    def->seclabels[def->nseclabels - 1] = seclabel;
> +
> +    return seclabel;
> +
> +no_memory:
> +    virReportOOMError();
> +    virSecurityLabelDefFree(seclabel);
> +    return NULL;
> +}

This first part would be better in a separate patch ...

> +
> +virSecurityDeviceLabelDefPtr
> +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model)
> +{
> +    virSecurityDeviceLabelDefPtr seclabel = NULL;
> +
> +    if (VIR_ALLOC(seclabel) < 0)
> +        goto no_memory;
> +
> +    if (model) {
> +        seclabel->model = strdup(model);
> +        if (seclabel->model == NULL)
> +            goto no_memory;
>       }
> +
> +    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0)
> +        goto no_memory;
> +
>       def->seclabels[def->nseclabels - 1] = seclabel;
>
>       return seclabel;
> +
> +no_memory:
> +    virReportOOMError();
> +    virSecurityDeviceLabelDefFree(seclabel);
> +    return NULL;
>   }

but I see why you did it.

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ce36003..9770ffb 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
>   virSecurityLabelDefPtr
>   virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
>
> +virSecurityDeviceLabelDefPtr
> +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model);
> +
>   typedef const char* (*virEventActionToStringFunc)(int type);
>   typedef int (*virEventActionFromStringFunc)(const char *type);
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b5e1a9a..511e923 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk,
>       if (ret == 1 && !disk_seclabel) {
>           /* If we failed to set a label, but virt_use_nfs let us
>            * proceed anyway, then we don't need to relabel later.  */
> -        if (VIR_ALLOC(disk_seclabel) < 0) {
> -            virReportOOMError();
> +        disk_seclabel =
> +            virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME);
> +        if (!disk_seclabel)
>               return -1;
> -        }
>           disk_seclabel->norelabel = true;
>           ret = 0;
>       }
>

ACK.

Peter