On Mon, 13 Jan 2025 at 17:38, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> From: Akihiko Odaki <akihiko.od...@daynix.com>
>
> Commit 03fcbd9dc508 ("qdev: Check for the availability of a hotplug
> controller before adding a device") says:
>
> > The qdev_unplug() function contains a g_assert(hotplug_ctrl)
> > statement, so QEMU crashes when the user tries to device_add +
> > device_del a device that does not have a corresponding hotplug
> > controller.
>
> > The code in qdev_device_add() already checks whether the bus has a
> > proper hotplug controller, but for devices that do not have a
> > corresponding bus, here is no appropriate check available yet. In that
> > case we should check whether the machine itself provides a suitable
> > hotplug controller and refuse to plug the device if none is available.
>
> However, it forgot to add the corresponding check to qdev_unplug().
>
> Check the machine hotplug handler once in the common
> qdev_hotplug_unplug_allowed_common() helper so both hotplug
> and hot-unplug path are covered.
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index e27d25c5859..861c25c855f 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -684,17 +684,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
> dev = qdev_new(driver);
>
> /* Check whether the hotplug is allowed by the machine */
> - if (phase_check(PHASE_MACHINE_READY)) {
> - if (!qdev_hotplug_allowed(dev, bus, errp)) {
> - goto err_del_dev;
> - }
> -
> - if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
> - /* No bus, no machine hotplug handler --> device is not
> hotpluggable */
> - error_setg(errp, "Device '%s' can not be hotplugged on this
> machine",
> - driver);
> - goto err_del_dev;
> - }
> + if (phase_check(PHASE_MACHINE_READY) &&
> + !qdev_hotplug_allowed(dev, bus, errp)) {
> + goto err_del_dev;
> }
>
Hi; Coverity notices a NULL-check related issue as a
result of this change. Here we add a call to
qdev_hotplug_allowed() that assumes that dev is not NULL.
But in the code at err_del_dev we do check whether
dev is NULL. So either this code is wrong (and could be
dereferencing NULL) or the code at err_del_dev is
doing a pointless NULL check.
Since qdev_new() can't fail, it's the err_del_dev
code that's wrong, I think.
thanks
-- PMM
