Message ID | 20210505135516.21097-1-peter.maydell@linaro.org |
---|---|
State | New |
Headers | show |
Series | tests/qtest/npcm7xx_pwm-test.c: Avoid g_assert_true() for non-test assertions | expand |
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>
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; }
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(-)