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