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@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