[PATCH] system/qdev: Remove pointless NULL check in qdev_device_add_from_qdict

[PATCH] system/qdev: Remove pointless NULL check in qdev_device_add_from_qdict

[Philippe Mathieu-Daudé]

Coverity reported a unnecessary NULL check:

  qemu/system/qdev-monitor.c: 720 in qdev_device_add_from_qdict()
  683     /* create device */
  684     dev = qdev_new(driver);
  ...
  719     err_del_dev:
  >>>     CID 1590192:  Null pointer dereferences  (REVERSE_INULL)
  >>>     Null-checking "dev" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
  720         if (dev) {
  721             object_unparent(OBJECT(dev));
  722             object_unref(OBJECT(dev));
  723         }
  724         return NULL;
  725     }

Indeed, unlike qdev_try_new() which can return NULL,
qdev_new() always returns a heap pointer (or aborts).

Remove the unnecessary assignment and check.

Fixes: f3a85056569 ("qdev/qbus: add hidden device support")
Resolves: Coverity CID 1590192 (Null pointer dereferences)
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 system/qdev-monitor.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 861c25c855f..01d4b007322 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -628,7 +628,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     DeviceClass *dc;
     const char *driver, *path;
     char *id;
-    DeviceState *dev = NULL;
+    DeviceState *dev;
     BusState *bus = NULL;
     QDict *properties;
 
@@ -717,10 +717,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     return dev;
 
 err_del_dev:
-    if (dev) {
-        object_unparent(OBJECT(dev));
-        object_unref(OBJECT(dev));
-    }
+    object_unparent(OBJECT(dev));
+    object_unref(OBJECT(dev));
+
     return NULL;
 }
 
-- 
2.47.1

Re: [PATCH] system/qdev: Remove pointless NULL check in qdev_device_add_from_qdict

[Richard Henderson]

On 2/10/25 04:10, Philippe Mathieu-Daudé wrote:
> Coverity reported a unnecessary NULL check:
> 
>    qemu/system/qdev-monitor.c: 720 in qdev_device_add_from_qdict()
>    683     /* create device */
>    684     dev = qdev_new(driver);
>    ...
>    719     err_del_dev:
>    >>>     CID 1590192:  Null pointer dereferences  (REVERSE_INULL)
>    >>>     Null-checking "dev" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
>    720         if (dev) {
>    721             object_unparent(OBJECT(dev));
>    722             object_unref(OBJECT(dev));
>    723         }
>    724         return NULL;
>    725     }
> 
> Indeed, unlike qdev_try_new() which can return NULL,
> qdev_new() always returns a heap pointer (or aborts).
> 
> Remove the unnecessary assignment and check.
> 
> Fixes: f3a85056569 ("qdev/qbus: add hidden device support")
> Resolves: Coverity CID 1590192 (Null pointer dereferences)
> Suggested-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   system/qdev-monitor.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH] system/qdev: Remove pointless NULL check in qdev_device_add_from_qdict

[Peter Maydell]

On Mon, 10 Feb 2025 at 12:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Coverity reported a unnecessary NULL check:
>
>   qemu/system/qdev-monitor.c: 720 in qdev_device_add_from_qdict()
>   683     /* create device */
>   684     dev = qdev_new(driver);
>   ...
>   719     err_del_dev:
>   >>>     CID 1590192:  Null pointer dereferences  (REVERSE_INULL)
>   >>>     Null-checking "dev" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
>   720         if (dev) {
>   721             object_unparent(OBJECT(dev));
>   722             object_unref(OBJECT(dev));
>   723         }
>   724         return NULL;
>   725     }
>
> Indeed, unlike qdev_try_new() which can return NULL,
> qdev_new() always returns a heap pointer (or aborts).
>
> Remove the unnecessary assignment and check.
>
> Fixes: f3a85056569 ("qdev/qbus: add hidden device support")
> Resolves: Coverity CID 1590192 (Null pointer dereferences)
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Looks like this got reviewed but never picked up by anybody.
I'll add it to my target-arm.next tree.

thanks
-- PMM

Re: [PATCH] system/qdev: Remove pointless NULL check in qdev_device_add_from_qdict

[Philippe Mathieu-Daudé]

On 10/7/25 16:48, Peter Maydell wrote:
> On Mon, 10 Feb 2025 at 12:10, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Coverity reported a unnecessary NULL check:
>>
>>    qemu/system/qdev-monitor.c: 720 in qdev_device_add_from_qdict()
>>    683     /* create device */
>>    684     dev = qdev_new(driver);
>>    ...
>>    719     err_del_dev:
>>    >>>     CID 1590192:  Null pointer dereferences  (REVERSE_INULL)
>>    >>>     Null-checking "dev" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
>>    720         if (dev) {
>>    721             object_unparent(OBJECT(dev));
>>    722             object_unref(OBJECT(dev));
>>    723         }
>>    724         return NULL;
>>    725     }
>>
>> Indeed, unlike qdev_try_new() which can return NULL,
>> qdev_new() always returns a heap pointer (or aborts).
>>
>> Remove the unnecessary assignment and check.
>>
>> Fixes: f3a85056569 ("qdev/qbus: add hidden device support")
>> Resolves: Coverity CID 1590192 (Null pointer dereferences)
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Looks like this got reviewed but never picked up by anybody.
> I'll add it to my target-arm.next tree.

Thanks!