https://bugzilla.redhat.com/show_bug.cgi?id=1141621 The crash in this case was because the qemu-attach code did not create aliases for qemu cli args. When the detach-interface code was called it assumed aliases were set resulting in a core when dereferencing the still NULL alias. Adding a call to qemuAssignDeviceAliases() resolves the path for qemu-attach; however, to prevent future issues an additional check for a NULL value is made and an error provided in the deatch path. Add some more verbiage to the virsh man page. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- v1 is here: http://www.redhat.com/archives/libvir-list/2014-September/msg01331.html Changes since v1: - Add the call to qemuAssignDeviceAliases() in qemuDomainQemuAttach(). - Move the check for NULL alias inside the CAPS_DEVICE check and emit an error rather than trying to remove as an "else" condition. src/qemu/qemu_driver.c | 3 +++ src/qemu/qemu_hotplug.c | 7 +++++++ tools/virsh.pod | 5 +++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 117138a..ef4ecd2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14746,6 +14746,9 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, if (qemuCanonicalizeMachine(def, qemuCaps) < 0) goto cleanup; + if (qemuAssignDeviceAliases(def, qemuCaps) < 0) + goto cleanup; + if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) goto cleanup; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d631887..daebe82 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3521,6 +3521,13 @@ qemuDomainDetachNetDevice(virConnectPtr conn, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + if (!detach->info.alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device alias not found: cannot delete the " + "net device")); + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, detach, NULL, "detach", false); diff --git a/tools/virsh.pod b/tools/virsh.pod index eae9195..bd17f68 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3698,8 +3698,9 @@ using the UNIX driver. Ideally the process will also have had the Not all functions of libvirt are expected to work reliably after attaching to an externally launched QEMU process. There may be -issues with the guest ABI changing upon migration, and hotunplug -may not work. +issues with the guest ABI changing upon migration and device hotplug +or hotunplug may not work. The attached environment should be considered +primarily read-only. =item B<qemu-monitor-command> I<domain> { [I<--hmp>] | [I<--pretty>] } I<command>...

On 25.09.2014 17:00, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1141621 > > The crash in this case was because the qemu-attach code did not create > aliases for qemu cli args. When the detach-interface code was called > it assumed aliases were set resulting in a core when dereferencing the > still NULL alias. > > Adding a call to qemuAssignDeviceAliases() resolves the path for > qemu-attach; however, to prevent future issues an additional check > for a NULL value is made and an error provided in the deatch path. > > Add some more verbiage to the virsh man page. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > > v1 is here: > http://www.redhat.com/archives/libvir-list/2014-September/msg01331.html > > Changes since v1: > - Add the call to qemuAssignDeviceAliases() in qemuDomainQemuAttach(). > - Move the check for NULL alias inside the CAPS_DEVICE check and emit > an error rather than trying to remove as an "else" condition. > > src/qemu/qemu_driver.c | 3 +++ > src/qemu/qemu_hotplug.c | 7 +++++++ > tools/virsh.pod | 5 +++-- > 3 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 117138a..ef4ecd2 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -14746,6 +14746,9 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, > if (qemuCanonicalizeMachine(def, qemuCaps) < 0) > goto cleanup; > > + if (qemuAssignDeviceAliases(def, qemuCaps) < 0) > + goto cleanup; > + > if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0) > goto cleanup; > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index d631887..daebe82 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -3521,6 +3521,13 @@ qemuDomainDetachNetDevice(virConnectPtr conn, > > qemuDomainObjEnterMonitor(driver, vm); > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > + if (!detach->info.alias) { > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("device alias not found: cannot delete the " > + "net device")); > + qemuDomainObjExitMonitor(driver, vm); > + goto cleanup; > + } Is there a reason to not check this upfront rather than this late in the process? And I don't think that net devices are the only thing affected here. For instance a virtio disks seems vulnerable too (well, at first glance on the code).

> if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { The other possibility would be to make qemuMonitorDelDevice fail on @devalias == NULL. > qemuDomainObjExitMonitor(driver, vm); > virDomainAuditNet(vm, detach, NULL, "detach", false); > diff --git a/tools/virsh.pod b/tools/virsh.pod > index eae9195..bd17f68 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -3698,8 +3698,9 @@ using the UNIX driver. Ideally the process will also have had the > > Not all functions of libvirt are expected to work reliably after > attaching to an externally launched QEMU process. There may be > -issues with the guest ABI changing upon migration, and hotunplug > -may not work. > +issues with the guest ABI changing upon migration and device hotplug > +or hotunplug may not work. The attached environment should be considered > +primarily read-only. > > =item B<qemu-monitor-command> I<domain> { [I<--hmp>] | [I<--pretty>] } > I<command>... > Michal