In the glib API, the distinction between g_assert() and
g_assert_true() is that the former is for "bug, terminate the
application" and the latter is for "test check, on failure either
terminate or just mark the testcase as failed".  For QEMU, g_assert()
is always fatal, so code can assume that if the assertion fails
execution does not proceed, but this is not true of g_assert_true().

In npcm7xx_pwm-test, the pwm_index() and pwm_module_index() functions
include some assertions that are just guarding against possible bugs
in the test code that might lead us to out-of-bounds array accesses.
These should use g_assert() because they aren't part of what the test
is testing and the code does not correctly handle the case where the
condition was false.

This fixes some Coverity issues where Coverity knows that
g_assert_true() can continue when the condition is false and
complains about the possible array overrun at various callsites.

Fixes: Coverity CID 1442340, 1442341, 1442343, 1442344, 1442345, 1442346
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/npcm7xx_pwm-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/npcm7xx_pwm-test.c b/tests/qtest/npcm7xx_pwm-test.c
index bd15a1c294b..a54fd70d273 100644
--- a/tests/qtest/npcm7xx_pwm-test.c
+++ b/tests/qtest/npcm7xx_pwm-test.c
@@ -201,7 +201,7 @@ static int pwm_module_index(const PWMModule *module)
 {
     ptrdiff_t diff = module - pwm_module_list;
 
-    g_assert_true(diff >= 0 && diff < ARRAY_SIZE(pwm_module_list));
+    g_assert(diff >= 0 && diff < ARRAY_SIZE(pwm_module_list));
 
     return diff;
 }
@@ -211,7 +211,7 @@ static int pwm_index(const PWM *pwm)
 {
     ptrdiff_t diff = pwm - pwm_list;
 
-    g_assert_true(diff >= 0 && diff < ARRAY_SIZE(pwm_list));
+    g_assert(diff >= 0 && diff < ARRAY_SIZE(pwm_list));
 
     return diff;
 }
-- 
2.20.1

On 05/05/2021 15.55, Peter Maydell wrote:
> In the glib API, the distinction between g_assert() and
> g_assert_true() is that the former is for "bug, terminate the
> application" and the latter is for "test check, on failure either
> terminate or just mark the testcase as failed".  For QEMU, g_assert()
> is always fatal, so code can assume that if the assertion fails
> execution does not proceed, but this is not true of g_assert_true().
> 
> In npcm7xx_pwm-test, the pwm_index() and pwm_module_index() functions
> include some assertions that are just guarding against possible bugs
> in the test code that might lead us to out-of-bounds array accesses.
> These should use g_assert() because they aren't part of what the test
> is testing and the code does not correctly handle the case where the
> condition was false.
> 
> This fixes some Coverity issues where Coverity knows that
> g_assert_true() can continue when the condition is false and
> complains about the possible array overrun at various callsites.
> 
> Fixes: Coverity CID 1442340, 1442341, 1442343, 1442344, 1442345, 1442346
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   tests/qtest/npcm7xx_pwm-test.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/npcm7xx_pwm-test.c b/tests/qtest/npcm7xx_pwm-test.c
> index bd15a1c294b..a54fd70d273 100644
> --- a/tests/qtest/npcm7xx_pwm-test.c
> +++ b/tests/qtest/npcm7xx_pwm-test.c
> @@ -201,7 +201,7 @@ static int pwm_module_index(const PWMModule *module)
>   {
>       ptrdiff_t diff = module - pwm_module_list;
>   
> -    g_assert_true(diff >= 0 && diff < ARRAY_SIZE(pwm_module_list));
> +    g_assert(diff >= 0 && diff < ARRAY_SIZE(pwm_module_list));
>   
>       return diff;
>   }
> @@ -211,7 +211,7 @@ static int pwm_index(const PWM *pwm)
>   {
>       ptrdiff_t diff = pwm - pwm_list;
>   
> -    g_assert_true(diff >= 0 && diff < ARRAY_SIZE(pwm_list));
> +    g_assert(diff >= 0 && diff < ARRAY_SIZE(pwm_list));
>   
>       return diff;
>   }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

On Wed, May 5, 2021 at 6:55 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the glib API, the distinction between g_assert() and
> g_assert_true() is that the former is for "bug, terminate the
> application" and the latter is for "test check, on failure either
> terminate or just mark the testcase as failed".  For QEMU, g_assert()
> is always fatal, so code can assume that if the assertion fails
> execution does not proceed, but this is not true of g_assert_true().
>
> In npcm7xx_pwm-test, the pwm_index() and pwm_module_index() functions
> include some assertions that are just guarding against possible bugs
> in the test code that might lead us to out-of-bounds array accesses.
> These should use g_assert() because they aren't part of what the test
> is testing and the code does not correctly handle the case where the
> condition was false.
>
> This fixes some Coverity issues where Coverity knows that
> g_assert_true() can continue when the condition is false and
> complains about the possible array overrun at various callsites.
>
> Fixes: Coverity CID 1442340, 1442341, 1442343, 1442344, 1442345, 1442346
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>

On Wed, May 5, 2021 at 7:09 AM Havard Skinnemoen <hskinnemoen@google.com>
wrote:

> On Wed, May 5, 2021 at 6:55 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > In the glib API, the distinction between g_assert() and
> > g_assert_true() is that the former is for "bug, terminate the
> > application" and the latter is for "test check, on failure either
> > terminate or just mark the testcase as failed".  For QEMU, g_assert()
> > is always fatal, so code can assume that if the assertion fails
> > execution does not proceed, but this is not true of g_assert_true().
> >
> > In npcm7xx_pwm-test, the pwm_index() and pwm_module_index() functions
> > include some assertions that are just guarding against possible bugs
> > in the test code that might lead us to out-of-bounds array accesses.
> > These should use g_assert() because they aren't part of what the test
> > is testing and the code does not correctly handle the case where the
> > condition was false.
> >
> > This fixes some Coverity issues where Coverity knows that
> > g_assert_true() can continue when the condition is false and
> > complains about the possible array overrun at various callsites.
> >
> > Fixes: Coverity CID 1442340, 1442341, 1442343, 1442344, 1442345, 1442346
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
>
Reviewed-by: Hao Wu <wuhaotsh@google.com>