[libvirt] [PATCH 1/2] security: DAC: fix the transaction model's list append

[Michal Privoznik]

On 01/17/2017 02:56 PM, Erik Skultety wrote:
> The problem is in the way how the list item is created prior to
> appending it to the transaction list - the @path attribute is just a
> shallow copy instead of deep copy of the hostdev device's path.
> Unfortunately, the hostdev devices from which the @path is extracted, in
> order to add them into the transaction list, are only temporary and
> freed before the buildup of the qemu namespace, thus making the @path
> attribute in the transaction list NULL, causing 'permission denied' or
> 'double free' or 'unknown cause' errors.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/security/security_dac.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index d457e6a..d7a2de4 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -71,7 +71,7 @@ struct _virSecurityDACCallbackData {
>  typedef struct _virSecurityDACChownItem virSecurityDACChownItem;
>  typedef virSecurityDACChownItem *virSecurityDACChownItemPtr;
>  struct _virSecurityDACChownItem {
> -    const char *path;
> +    char *path;
>      const virStorageSource *src;
>      uid_t uid;
>      gid_t gid;
> @@ -95,22 +95,32 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list,
>                                uid_t uid,
>                                gid_t gid)
>  {
> -    virSecurityDACChownItemPtr item;
> +    int ret = -1;
> +    char *tmp = NULL;
> +    virSecurityDACChownItemPtr item = NULL;
>  
>      if (VIR_ALLOC(item) < 0)
>          return -1;
>  
> -    item->path = path;
> +    if (VIR_STRDUP(tmp, path) < 0)
> +        goto cleanup;
> +
> +    item->path = tmp;
>      item->src = src;
>      item->uid = uid;
>      item->gid = gid;
>  
> -    if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) {
> -        VIR_FREE(item);
> -        return -1;
> -    }
> +    if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
> +        goto cleanup;
>  
> -    return 0;
> +    tmp = NULL;
> +    item = NULL;

This 'item = NULL' is not needed. VIR_APPEND_ELEMENT sets @item to NULL
upon successful return. But I agree that it is hard to spot.

> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(tmp);
> +    VIR_FREE(item);
> +    return ret;
>  }
>  
>  static void
> @@ -122,8 +132,10 @@ virSecurityDACChownListFree(void *opaque)
>      if (!list)
>          return;
>  
> -    for (i = 0; i < list->nItems; i++)
> +    for (i = 0; i < list->nItems; i++) {
> +        VIR_FREE(list->items[i]->path);
>          VIR_FREE(list->items[i]);
> +    }
>      VIR_FREE(list);
>  }
>  
> 

ACK and safe for the freeze.

Michal