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@xxxxxxxxxx> > --- > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
