On 09/17/2014 06:45 AM, Ján Tomko wrote:

Commit f36a94f introduced a double free on all success paths in qemuSharedDeviceEntryInsert.

Only call qemuSharedDeviceEntryFree on the error path and set entry to NULL before jumping there if the entry already is in the hash table.

https://bugzilla.redhat.com/show_bug.cgi?id=1142722 --- src/qemu/qemu_conf.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac10b64..adc6caf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1011,38 +1011,36 @@ qemuSharedDeviceEntryInsert(virQEMUDriverPtr driver, const char *name) { qemuSharedDeviceEntry *entry = NULL; - int ret = -1;

if ((entry = virHashLookup(driver->sharedDevices, key))) { /* Nothing to do if the shared scsi host device is already * recorded in the table. */ - if (qemuSharedDeviceEntryDomainExists(entry, name, NULL)) { - ret = 0; - goto cleanup; + if (!qemuSharedDeviceEntryDomainExists(entry, name, NULL)) { + if (VIR_EXPAND_N(entry->domains, entry->ref, 1) < 0 || + VIR_STRDUP(entry->domains[entry->ref - 1], name) < 0) { + /* entry is owned by the hash table here */ + entry = NULL;

[1] Assigning to NULL causes an issue

+ goto error; + } } - - if (VIR_EXPAND_N(entry->domains, entry->ref, 1) < 0 || - VIR_STRDUP(entry->domains[entry->ref - 1], name) < 0) - goto cleanup; } else { if (VIR_ALLOC(entry) < 0 || VIR_ALLOC_N(entry->domains, 1) < 0 || VIR_STRDUP(entry->domains[0], name) < 0) - goto cleanup; + goto error;

entry->ref = 1;

if (virHashAddEntry(driver->sharedDevices, key, entry)) - goto cleanup; + goto error; }

- ret = 0; + return 0;

- cleanup: + error: qemuSharedDeviceEntryFree(entry, NULL); [1] Because this is prototyped as:

void qemuSharedDeviceEntryFree(void *payload, const void *name) ATTRIBUTE_NONNULL(1); Coverity gives us a warning when entry = NULL... It's solveable by either allowing NULL for the function or only calling if (entry) ACK as long as we handle in some manner. John

- - return ret; + return -1; }