[PATCH] tests: unit: add NULL-pointer check

[PATCH] tests: unit: add NULL-pointer check

[Paolo Bonzini]

In CID 1432593, Coverity complains that the result of qdict_crumple()
might leak if it is not a dictionary.  This is not a practical concern
since the test would fail immediately with a NULL pointer dereference
in qdict_size().

However, it is not nice to depend on qdict_size() crashing, so add an
explicit assertion that that the crumpled object was indeed a dictionary.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/unit/check-block-qdict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/unit/check-block-qdict.c b/tests/unit/check-block-qdict.c
index 5a25825093..751c58e737 100644
--- a/tests/unit/check-block-qdict.c
+++ b/tests/unit/check-block-qdict.c
@@ -504,7 +504,7 @@ static void qdict_crumple_test_empty(void)
     src = qdict_new();
 
     dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
-
+    g_assert(dst);
     g_assert_cmpint(qdict_size(dst), ==, 0);
 
     qobject_unref(src);
-- 
2.37.2

Re: [PATCH] tests: unit: add NULL-pointer check

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> In CID 1432593, Coverity complains that the result of qdict_crumple()
> might leak if it is not a dictionary.  This is not a practical concern
> since the test would fail immediately with a NULL pointer dereference
> in qdict_size().
>
> However, it is not nice to depend on qdict_size() crashing, so add an
> explicit assertion that that the crumpled object was indeed a dictionary.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH] tests: unit: add NULL-pointer check

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> In CID 1432593, Coverity complains that the result of qdict_crumple()
> might leak if it is not a dictionary.  This is not a practical concern
> since the test would fail immediately with a NULL pointer dereference
> in qdict_size().
>
> However, it is not nice to depend on qdict_size() crashing, so add an
> explicit assertion that that the crumpled object was indeed a dictionary.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/unit/check-block-qdict.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/unit/check-block-qdict.c b/tests/unit/check-block-qdict.c
> index 5a25825093..751c58e737 100644
> --- a/tests/unit/check-block-qdict.c
> +++ b/tests/unit/check-block-qdict.c
> @@ -504,7 +504,7 @@ static void qdict_crumple_test_empty(void)
>      src = qdict_new();
>  
>      dst = qobject_to(QDict, qdict_crumple(src, &error_abort));
> -
> +    g_assert(dst);
>      g_assert_cmpint(qdict_size(dst), ==, 0);
>  
>      qobject_unref(src);

First, I'm fine with the patch, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Next, permit me a few words on writing tests.  For me, a unit test fails
by crashing.  Crashing with a nice message is optional.  The more likely
the failure, the more useful is niceness.  Complete niceness is
impossible --- if we could predict all crashes, we wouldn't need tests.
Trying to push niceness can be overly verbose.  Thus, judgement calls,
and matters of taste.

Wanting to mollify Coverity is a valid argument.

Re: [PATCH] tests: unit: add NULL-pointer check

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

Il mar 6 set 2022, 07:01 Markus Armbruster <armbru@redhat.com> ha scritto:

> Next, permit me a few words on writing tests.  For me, a unit test fails
> by crashing.  Crashing with a nice message is optional.  The more likely
> the failure, the more useful is niceness.  Complete niceness is
> impossible --- if we could predict all crashes, we wouldn't need tests.
> Trying to push niceness can be overly verbose.  Thus, judgement calls,
> and matters of taste.
>

I agree; however, *relying* on a crash for correctness of the test is not
great. Part of the test here is checking that an empty qdict_crumple
returns a dictionary and not, say, a list. The newly-added assertion avoids
that two wrongs end up making a right: if qobject_check_type somehow failed
to identify the dictionary and returned (QDict *) obj, qdict_size would not
crash.

Unlikely as it is, it's nicer to spell out the postconditions that the test
is checking.

Paolo



> Wanting to mollify Coverity is a valid argument.
>
>

[-- Attachment #2: Type: text/html, Size: 1670 bytes --]