[libvirt] [PATCH 1/4] security: Grab a reference to virSecurityManager for transactions

[John Ferlan]

On 9/21/18 5:29 AM, Michal Privoznik wrote:
> This shouldn't be needed per-se. Security manager shouldn't
> disappear during transactions - it's immutable. However, it
> doesn't hurt to grab a reference either - transaction code uses
> it after all.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_dac.c     | 5 +++--
>  src/security/security_selinux.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

FWIW: I agree w/ Marc's assessment.  You need a patch 0.5 ;-) to add the
VIR_FREE(list->items) for selinux.  It should reference commit ca25026


> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 2dbaf29ff5..5aea386e7c 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -141,6 +141,7 @@ virSecurityDACChownListFree(void *opaque)
>          VIR_FREE(list->items[i]);
>      }
>      VIR_FREE(list->items);
> +    virObjectUnref(list->manager);
>      VIR_FREE(list);
>  }
>  
> @@ -511,12 +512,12 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
>      if (VIR_ALLOC(list) < 0)
>          return -1;
>  
> -    list->manager = mgr;
> +    list->manager = virObjectRef(mgr);

If you move ^^^ to below vvv, then I think the VIR_FREE could still
apply. Realistically all that's happening by calling ListFree is the
Unref of list->manager.  Same for _selinux.

In fact, that'd probably be the more proper order with the Ref(mgr)
being done.

With all that,

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

I also assume you can add the VIR_FREE(list->items) to _selinux with an
appropriate commit message as a pre-patch and that too has my R-By.

>  
>      if (virThreadLocalSet(&chownList, list) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Unable to set thread local variable"));
> -        VIR_FREE(list);
> +        virSecurityDACChownListFree(list);
>          return -1;
>      }
>  
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 056637e4cb..31e42afee7 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -156,6 +156,7 @@ virSecuritySELinuxContextListFree(void *opaque)
>      for (i = 0; i < list->nItems; i++)
>          virSecuritySELinuxContextItemFree(list->items[i]);
>  
> +    virObjectUnref(list->manager);
>      VIR_FREE(list);
>  }
>  
> @@ -1054,12 +1055,12 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr)
>      if (VIR_ALLOC(list) < 0)
>          return -1;
>  
> -    list->manager = mgr;
> +    list->manager = virObjectRef(mgr);
>  
>      if (virThreadLocalSet(&contextList, list) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Unable to set thread local variable"));
> -        VIR_FREE(list);
> +        virSecuritySELinuxContextListFree(list);
>          return -1;
>      }
>  
>