[PATCH-for-5.2? 0/2] two qtest bugfix

[PATCH-for-5.2? 0/2] two qtest bugfix

[Chen Qun]

Hi folks,
  There are two bug caused by recent testcase code merge.
EulerRobot found them, and this series fixed them.

Thanks,
Chen Qun

Chen Qun (2):
  tests/qtest: variable defined by g_autofree need to be initialized
  tests/qtest: fix memleak in npcm7xx_watchdog_timer-test

 tests/qtest/npcm7xx_timer-test.c          | 8 +++-----
 tests/qtest/npcm7xx_watchdog_timer-test.c | 6 ++++--
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.23.0

[PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Chen Qun]

According to the glib function requirements, we need initialise
 the variable. Otherwise there will be compilation warnings:

glib-autocleanups.h:28:3: warning: ‘full_name’ may be
used uninitialized in this function [-Wmaybe-uninitialized]
   28 |   g_free (*pp);
      |   ^~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
 tests/qtest/npcm7xx_timer-test.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c
index f08b0cd62a..83774a5b90 100644
--- a/tests/qtest/npcm7xx_timer-test.c
+++ b/tests/qtest/npcm7xx_timer-test.c
@@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data)
  */
 static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn)
 {
-    g_autofree char *full_name;
-
-    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
-                                tim_index(td->tim), timer_index(td->timer),
-                                name);
+    g_autofree char *full_name = g_strdup_printf(
+        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
+        timer_index(td->timer), name);
     qtest_add_data_func(full_name, td, fn);
 }
 
-- 
2.23.0

Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Philippe Mathieu-Daudé]

On 11/18/20 12:56 PM, Chen Qun wrote:
> According to the glib function requirements, we need initialise
>  the variable. Otherwise there will be compilation warnings:
> 
> glib-autocleanups.h:28:3: warning: ‘full_name’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
>    28 |   g_free (*pp);
>       |   ^~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
>  tests/qtest/npcm7xx_timer-test.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c
> index f08b0cd62a..83774a5b90 100644
> --- a/tests/qtest/npcm7xx_timer-test.c
> +++ b/tests/qtest/npcm7xx_timer-test.c
> @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data)
>   */
>  static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn)
>  {

Or:

> -    g_autofree char *full_name;
  +    g_autofree char *full_name = NULL;

> -
> -    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> -                                tim_index(td->tim), timer_index(td->timer),
> -                                name);
> +    g_autofree char *full_name = g_strdup_printf(
> +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> +        timer_index(td->timer), name);
>      qtest_add_data_func(full_name, td, fn);
>  }
>  
> 

RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Chenqun (kuhn)]

> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: Wednesday, November 18, 2020 8:14 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: lvivier@redhat.com; peter.maydell@linaro.org; thuth@redhat.com;
> Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> hskinnemoen@google.com; wuhaotsh@google.com; Euler Robot
> <euler.robot@huawei.com>
> Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree
> need to be initialized
> 
> On 11/18/20 12:56 PM, Chen Qun wrote:
> > According to the glib function requirements, we need initialise  the
> > variable. Otherwise there will be compilation warnings:
> >
> > glib-autocleanups.h:28:3: warning: ‘full_name’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> >    28 |   g_free (*pp);
> >       |   ^~~~~~~~~~~~
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > ---
> >  tests/qtest/npcm7xx_timer-test.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/npcm7xx_timer-test.c
> > b/tests/qtest/npcm7xx_timer-test.c
> > index f08b0cd62a..83774a5b90 100644
> > --- a/tests/qtest/npcm7xx_timer-test.c
> > +++ b/tests/qtest/npcm7xx_timer-test.c
> > @@ -512,11 +512,9 @@ static void
> test_disable_on_expiration(gconstpointer test_data)
> >   */
> >  static void tim_add_test(const char *name, const TestData *td,
> > GTestDataFunc fn)  {
> 
> Or:
> 
> > -    g_autofree char *full_name;
>   +    g_autofree char *full_name = NULL;
> 
Yes, this also meets the glib requirements.
But, the assignment statement following is not complex, so we could do both of variable definition and assignment in a statement.

Thanks,
Chen Qun
> > -
> > -    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > -                                tim_index(td->tim),
> timer_index(td->timer),
> > -                                name);
> > +    g_autofree char *full_name = g_strdup_printf(
> > +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > +        timer_index(td->timer), name);
> >      qtest_add_data_func(full_name, td, fn);  }
> >
> >

Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Thomas Huth]

On 18/11/2020 13.13, Philippe Mathieu-Daudé wrote:
> On 11/18/20 12:56 PM, Chen Qun wrote:
>> According to the glib function requirements, we need initialise
>>  the variable. Otherwise there will be compilation warnings:
>>
>> glib-autocleanups.h:28:3: warning: ‘full_name’ may be
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>    28 |   g_free (*pp);
>>       |   ^~~~~~~~~~~~
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>>  tests/qtest/npcm7xx_timer-test.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c
>> index f08b0cd62a..83774a5b90 100644
>> --- a/tests/qtest/npcm7xx_timer-test.c
>> +++ b/tests/qtest/npcm7xx_timer-test.c
>> @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data)
>>   */
>>  static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn)
>>  {
> 
> Or:
> 
>> -    g_autofree char *full_name;
>   +    g_autofree char *full_name = NULL;

I think we better go with the current version of the patch - otherwise a
supersmart new compiler version might warn again that the NULL that we
assign here is never used...

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

Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Havard Skinnemoen]

On Wed, Nov 18, 2020 at 3:57 AM Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> According to the glib function requirements, we need initialise
>  the variable. Otherwise there will be compilation warnings:
>
> glib-autocleanups.h:28:3: warning: ‘full_name’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
>    28 |   g_free (*pp);
>       |   ^~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

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

I'd be totally fine with Philippe's suggestion too.

Thanks!

> ---
>  tests/qtest/npcm7xx_timer-test.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/npcm7xx_timer-test.c b/tests/qtest/npcm7xx_timer-test.c
> index f08b0cd62a..83774a5b90 100644
> --- a/tests/qtest/npcm7xx_timer-test.c
> +++ b/tests/qtest/npcm7xx_timer-test.c
> @@ -512,11 +512,9 @@ static void test_disable_on_expiration(gconstpointer test_data)
>   */
>  static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn)
>  {
> -    g_autofree char *full_name;
> -
> -    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> -                                tim_index(td->tim), timer_index(td->timer),
> -                                name);
> +    g_autofree char *full_name = g_strdup_printf(
> +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> +        timer_index(td->timer), name);
>      qtest_add_data_func(full_name, td, fn);
>  }
>
> --
> 2.23.0
>

Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Peter Maydell]

On Wed, 18 Nov 2020 at 11:57, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> According to the glib function requirements, we need initialise
>  the variable. Otherwise there will be compilation warnings:
>
> glib-autocleanups.h:28:3: warning: ‘full_name’ may be
> used uninitialized in this function [-Wmaybe-uninitialized]
>    28 |   g_free (*pp);
>       |   ^~~~~~~~~~~~
>
>  static void tim_add_test(const char *name, const TestData *td, GTestDataFunc fn)
>  {
> -    g_autofree char *full_name;
> -
> -    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> -                                tim_index(td->tim), timer_index(td->timer),
> -                                name);
> +    g_autofree char *full_name = g_strdup_printf(
> +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> +        timer_index(td->timer), name);

Which compiler is so unintelligent that it cannot see that
a declaration immediately followed by an assignment must
always initialize the variable ???

thanks
-- PMM

RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Chenqun (kuhn)]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, November 19, 2020 4:26 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial
> <qemu-trivial@nongnu.org>; Hao Wu <wuhaotsh@google.com>; Havard
> Skinnemoen <hskinnemoen@google.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Thomas Huth <thuth@redhat.com>;
> Laurent Vivier <lvivier@redhat.com>; Euler Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree
> need to be initialized
> 
> On Wed, 18 Nov 2020 at 11:57, Chen Qun <kuhn.chenqun@huawei.com>
> wrote:
> >
> > According to the glib function requirements, we need initialise  the
> > variable. Otherwise there will be compilation warnings:
> >
> > glib-autocleanups.h:28:3: warning: ‘full_name’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> >    28 |   g_free (*pp);
> >       |   ^~~~~~~~~~~~
> >
> >  static void tim_add_test(const char *name, const TestData *td,
> > GTestDataFunc fn)  {
> > -    g_autofree char *full_name;
> > -
> > -    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > -                                tim_index(td->tim),
> timer_index(td->timer),
> > -                                name);
> > +    g_autofree char *full_name = g_strdup_printf(
> > +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > +        timer_index(td->timer), name);
> 
> Which compiler is so unintelligent that it cannot see that a declaration
> immediately followed by an assignment must always initialize the variable ???
> 
Hi Peter,
  Glib requires that all g_auto* macros must be initialized.
  https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree

Thanks,
Chen Qun

Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Peter Maydell]

On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn) <kuhn.chenqun@huawei.com> wrote:
>
> > -----Original Message-----
> > >  static void tim_add_test(const char *name, const TestData *td,
> > > GTestDataFunc fn)  {
> > > -    g_autofree char *full_name;
> > > -
> > > -    full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > > -                                tim_index(td->tim),
> > timer_index(td->timer),
> > > -                                name);
> > > +    g_autofree char *full_name = g_strdup_printf(
> > > +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > > +        timer_index(td->timer), name);
> >
> > Which compiler is so unintelligent that it cannot see that a declaration
> > immediately followed by an assignment must always initialize the variable ???
> >
> Hi Peter,
>   Glib requires that all g_auto* macros must be initialized.
>   https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree

Yes, and we initialize it with the "full_name = ..." line.
The g_autofree documentation says "this macro has similar constraints
as g_autoptr()", and the g_autoptr() documentation says
"You must initialise the variable in some way — either by use of an
initialiser or by ensuring that it is assigned to unconditionally
before it goes out of scope."

In this case the test code is doing the second of those two things.

thanks
-- PMM

RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Chenqun (kuhn)]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, November 19, 2020 4:39 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial
> <qemu-trivial@nongnu.org>; Hao Wu <wuhaotsh@google.com>; Havard
> Skinnemoen <hskinnemoen@google.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Thomas Huth <thuth@redhat.com>;
> Laurent Vivier <lvivier@redhat.com>; Euler Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree
> need to be initialized
> 
> On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> wrote:
> >
> > > -----Original Message-----
> > > >  static void tim_add_test(const char *name, const TestData *td,
> > > > GTestDataFunc fn)  {
> > > > -    g_autofree char *full_name;
> > > > -
> > > > -    full_name =
> g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > > > -                                tim_index(td->tim),
> > > timer_index(td->timer),
> > > > -                                name);
> > > > +    g_autofree char *full_name = g_strdup_printf(
> > > > +        "npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > > > +        timer_index(td->timer), name);
> > >
> > > Which compiler is so unintelligent that it cannot see that a
> > > declaration immediately followed by an assignment must always initialize
> the variable ???
> > >
> > Hi Peter,
> >   Glib requires that all g_auto* macros must be initialized.
> >
> > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html
> > #g-autofree
> 
> Yes, and we initialize it with the "full_name = ..." line.
> The g_autofree documentation says "this macro has similar constraints as
> g_autoptr()", and the g_autoptr() documentation says "You must initialise the
> variable in some way — either by use of an initialiser or by ensuring that it is
> assigned to unconditionally before it goes out of scope."
> 
> In this case the test code is doing the second of those two things.

Emm, maybe I didn't get it right. I've tried something as following:
There are three pieces of code complied in GCC9.3 and GCC7.3.
Code1：
   g_autofree char *full_name;
   full_name = g_strdup_printf("npcm7xx_timer");

Code2:
   g_autofree char *full_name = g_strdup_printf("npcm7xx_timer");

Code3：
   g_autofree char *full_name;
   full_name = NULL;

The result is as follows:
  Code1: An warnig is generated for GCC7.3 or GCC9.3.

  Code2 and Code3: no any warnig whether compiler is GCC7.3 or GCC9.3.

I cannot explain why the Code1 warning is generated. But it always generates warning on the GCC compiler.

Thanks,
Chen Qun

RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized

[Chenqun (kuhn)]

> > On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>
> > wrote:
> > >
> > > > -----Original Message-----
> > > > >  static void tim_add_test(const char *name, const TestData *td,
> > > > > GTestDataFunc fn)  {
> > > > > -    g_autofree char *full_name;
> > > > > -
> > > > > -    full_name =
> > g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > > > > -                                tim_index(td->tim),
> > > > timer_index(td->timer),
> > > > > -                                name);
> > > > > +    g_autofree char *full_name = g_strdup_printf(
> > > > > +        "npcm7xx_timer/tim[%d]/timer[%d]/%s",
> tim_index(td->tim),
> > > > > +        timer_index(td->timer), name);
> > > >
> > > > Which compiler is so unintelligent that it cannot see that a
> > > > declaration immediately followed by an assignment must always
> > > > initialize
> > the variable ???
> > > >
> > > Hi Peter,
> > >   Glib requires that all g_auto* macros must be initialized.
> > >
> > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.ht
> > > ml
> > > #g-autofree
> >
> > Yes, and we initialize it with the "full_name = ..." line.
> > The g_autofree documentation says "this macro has similar constraints
> > as g_autoptr()", and the g_autoptr() documentation says "You must
> > initialise the variable in some way — either by use of an initialiser
> > or by ensuring that it is assigned to unconditionally before it goes out of
> scope."
> >
> > In this case the test code is doing the second of those two things.
> 
> Emm, maybe I didn't get it right. I've tried something as following:
> There are three pieces of code complied in GCC9.3 and GCC7.3.
> Code1：
>    g_autofree char *full_name;
>    full_name = g_strdup_printf("npcm7xx_timer");
I guess the GCC thinks that g_strdup_printf() may fail to be executed. After the function fails to be executed, and not give a NULL return value.
If this occurs, g_autofree will still executes g_free(*pp).  So, an warning is generated in the Code1 case.

In the example code provided by g-autoptr() documentation, the value assignment statement of the 'dirname' variable uses g_variant_lookup_value().
If the g_variant_lookup_value() function fails to be executed, a NULL value is returned. So, this example code is safe.
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autoptr
> 
> Code2:
>    g_autofree char *full_name = g_strdup_printf("npcm7xx_timer");
In this case, if g_strdup_printf() fails, the variable definition also fails. So, the Code2 is safe.
> 
> Code3：
>    g_autofree char *full_name;
>    full_name = NULL;
> 
> The result is as follows:
>   Code1: An warnig is generated for GCC7.3 or GCC9.3.
> 
>   Code2 and Code3: no any warnig whether compiler is GCC7.3 or GCC9.3.
> 
> I cannot explain why the Code1 warning is generated. But it always generates
> warning on the GCC compiler.

The above analysis is just my own guess. That may not be the truth.

Thanks,
Chen Qun

[PATCH-for-5.2? 2/2] tests/qtest: fix memleak in npcm7xx_watchdog_timer-test

[Chen Qun]

Properly free resp for get_watchdog_action() to avoid memory leak.
ASAN shows memory leak stack:

Indirect leak of 12360 byte(s) in 3 object(s) allocated from:
    #0 0x7f41ab6cbd4e in __interceptor_calloc (/lib64/libasan.so.5+0x112d4e)
    #1 0x7f41ab4eaa50 in g_malloc0 (/lib64/libglib-2.0.so.0+0x55a50)
    #2 0x556487d5374b in qdict_new ../qobject/qdict.c:29
    #3 0x556487d65e1a in parse_object ../qobject/json-parser.c:318
    #4 0x556487d65cb6 in parse_pair ../qobject/json-parser.c:287
    #5 0x556487d65ebd in parse_object ../qobject/json-parser.c:343
    #6 0x556487d661d5 in json_parser_parse ../qobject/json-parser.c:580
    #7 0x556487d513df in json_message_process_token ../qobject/json-streamer.c:92
    #8 0x556487d63919 in json_lexer_feed_char ../qobject/json-lexer.c:313
    #9 0x556487d63d75 in json_lexer_feed ../qobject/json-lexer.c:350
    #10 0x556487d28b2a in qmp_fd_receive ../tests/qtest/libqtest.c:613
    #11 0x556487d2a16f in qtest_qmp_eventwait_ref ../tests/qtest/libqtest.c:827
    #12 0x556487d248e2 in get_watchdog_action ../tests/qtest/npcm7xx_watchdog_timer-test.c:94
    #13 0x556487d25765 in test_enabling_flags ../tests/qtest/npcm7xx_watchdog_timer-test.c:243

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
 tests/qtest/npcm7xx_watchdog_timer-test.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/npcm7xx_watchdog_timer-test.c b/tests/qtest/npcm7xx_watchdog_timer-test.c
index 54d5d6d8f2..3aae5a0438 100644
--- a/tests/qtest/npcm7xx_watchdog_timer-test.c
+++ b/tests/qtest/npcm7xx_watchdog_timer-test.c
@@ -204,6 +204,7 @@ static void test_enabling_flags(gconstpointer watchdog)
 {
     const Watchdog *wd = watchdog;
     QTestState *qts;
+    QDict *rsp;
 
     /* Neither WTIE or WTRE is set, no interrupt or reset should happen */
     qts = qtest_init("-machine quanta-gsj");
@@ -240,8 +241,9 @@ static void test_enabling_flags(gconstpointer watchdog)
     g_assert_false(qtest_get_irq(qts, wd->irq));
     qtest_clock_step(qts, watchdog_calculate_steps(RESET_CYCLES,
                 watchdog_prescaler(qts, wd)));
-    g_assert_false(strcmp(qdict_get_str(get_watchdog_action(qts), "action"),
-                "reset"));
+    rsp = get_watchdog_action(qts);
+    g_assert_false(strcmp(qdict_get_str(rsp, "action"), "reset"));
+    qobject_unref(rsp);
     qtest_qmp_eventwait(qts, "RESET");
     qtest_quit(qts);
 
-- 
2.23.0

Re: [PATCH-for-5.2? 2/2] tests/qtest: fix memleak in npcm7xx_watchdog_timer-test

[Havard Skinnemoen]

On Wed, Nov 18, 2020 at 3:57 AM Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Properly free resp for get_watchdog_action() to avoid memory leak.
> ASAN shows memory leak stack:
>
> Indirect leak of 12360 byte(s) in 3 object(s) allocated from:
>     #0 0x7f41ab6cbd4e in __interceptor_calloc (/lib64/libasan.so.5+0x112d4e)
>     #1 0x7f41ab4eaa50 in g_malloc0 (/lib64/libglib-2.0.so.0+0x55a50)
>     #2 0x556487d5374b in qdict_new ../qobject/qdict.c:29
>     #3 0x556487d65e1a in parse_object ../qobject/json-parser.c:318
>     #4 0x556487d65cb6 in parse_pair ../qobject/json-parser.c:287
>     #5 0x556487d65ebd in parse_object ../qobject/json-parser.c:343
>     #6 0x556487d661d5 in json_parser_parse ../qobject/json-parser.c:580
>     #7 0x556487d513df in json_message_process_token ../qobject/json-streamer.c:92
>     #8 0x556487d63919 in json_lexer_feed_char ../qobject/json-lexer.c:313
>     #9 0x556487d63d75 in json_lexer_feed ../qobject/json-lexer.c:350
>     #10 0x556487d28b2a in qmp_fd_receive ../tests/qtest/libqtest.c:613
>     #11 0x556487d2a16f in qtest_qmp_eventwait_ref ../tests/qtest/libqtest.c:827
>     #12 0x556487d248e2 in get_watchdog_action ../tests/qtest/npcm7xx_watchdog_timer-test.c:94
>     #13 0x556487d25765 in test_enabling_flags ../tests/qtest/npcm7xx_watchdog_timer-test.c:243
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

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

> ---
>  tests/qtest/npcm7xx_watchdog_timer-test.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/npcm7xx_watchdog_timer-test.c b/tests/qtest/npcm7xx_watchdog_timer-test.c
> index 54d5d6d8f2..3aae5a0438 100644
> --- a/tests/qtest/npcm7xx_watchdog_timer-test.c
> +++ b/tests/qtest/npcm7xx_watchdog_timer-test.c
> @@ -204,6 +204,7 @@ static void test_enabling_flags(gconstpointer watchdog)
>  {
>      const Watchdog *wd = watchdog;
>      QTestState *qts;
> +    QDict *rsp;
>
>      /* Neither WTIE or WTRE is set, no interrupt or reset should happen */
>      qts = qtest_init("-machine quanta-gsj");
> @@ -240,8 +241,9 @@ static void test_enabling_flags(gconstpointer watchdog)
>      g_assert_false(qtest_get_irq(qts, wd->irq));
>      qtest_clock_step(qts, watchdog_calculate_steps(RESET_CYCLES,
>                  watchdog_prescaler(qts, wd)));
> -    g_assert_false(strcmp(qdict_get_str(get_watchdog_action(qts), "action"),
> -                "reset"));
> +    rsp = get_watchdog_action(qts);
> +    g_assert_false(strcmp(qdict_get_str(rsp, "action"), "reset"));
> +    qobject_unref(rsp);
>      qtest_qmp_eventwait(qts, "RESET");
>      qtest_quit(qts);
>
> --
> 2.23.0
>

Re: [PATCH-for-5.2? 2/2] tests/qtest: fix memleak in npcm7xx_watchdog_timer-test

[Hao Wu]

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

On Wed, Nov 18, 2020 at 9:14 AM Havard Skinnemoen <hskinnemoen@google.com>
wrote:

> On Wed, Nov 18, 2020 at 3:57 AM Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >
> > Properly free resp for get_watchdog_action() to avoid memory leak.
> > ASAN shows memory leak stack:
> >
> > Indirect leak of 12360 byte(s) in 3 object(s) allocated from:
> >     #0 0x7f41ab6cbd4e in __interceptor_calloc
> (/lib64/libasan.so.5+0x112d4e)
> >     #1 0x7f41ab4eaa50 in g_malloc0 (/lib64/libglib-2.0.so.0+0x55a50)
> >     #2 0x556487d5374b in qdict_new ../qobject/qdict.c:29
> >     #3 0x556487d65e1a in parse_object ../qobject/json-parser.c:318
> >     #4 0x556487d65cb6 in parse_pair ../qobject/json-parser.c:287
> >     #5 0x556487d65ebd in parse_object ../qobject/json-parser.c:343
> >     #6 0x556487d661d5 in json_parser_parse ../qobject/json-parser.c:580
> >     #7 0x556487d513df in json_message_process_token
> ../qobject/json-streamer.c:92
> >     #8 0x556487d63919 in json_lexer_feed_char ../qobject/json-lexer.c:313
> >     #9 0x556487d63d75 in json_lexer_feed ../qobject/json-lexer.c:350
> >     #10 0x556487d28b2a in qmp_fd_receive ../tests/qtest/libqtest.c:613
> >     #11 0x556487d2a16f in qtest_qmp_eventwait_ref
> ../tests/qtest/libqtest.c:827
> >     #12 0x556487d248e2 in get_watchdog_action
> ../tests/qtest/npcm7xx_watchdog_timer-test.c:94
> >     #13 0x556487d25765 in test_enabling_flags
> ../tests/qtest/npcm7xx_watchdog_timer-test.c:243
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>
> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>

> > ---
> >  tests/qtest/npcm7xx_watchdog_timer-test.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qtest/npcm7xx_watchdog_timer-test.c
> b/tests/qtest/npcm7xx_watchdog_timer-test.c
> > index 54d5d6d8f2..3aae5a0438 100644
> > --- a/tests/qtest/npcm7xx_watchdog_timer-test.c
> > +++ b/tests/qtest/npcm7xx_watchdog_timer-test.c
> > @@ -204,6 +204,7 @@ static void test_enabling_flags(gconstpointer
> watchdog)
> >  {
> >      const Watchdog *wd = watchdog;
> >      QTestState *qts;
> > +    QDict *rsp;
> >
> >      /* Neither WTIE or WTRE is set, no interrupt or reset should happen
> */
> >      qts = qtest_init("-machine quanta-gsj");
> > @@ -240,8 +241,9 @@ static void test_enabling_flags(gconstpointer
> watchdog)
> >      g_assert_false(qtest_get_irq(qts, wd->irq));
> >      qtest_clock_step(qts, watchdog_calculate_steps(RESET_CYCLES,
> >                  watchdog_prescaler(qts, wd)));
> > -    g_assert_false(strcmp(qdict_get_str(get_watchdog_action(qts),
> "action"),
> > -                "reset"));
> > +    rsp = get_watchdog_action(qts);
> > +    g_assert_false(strcmp(qdict_get_str(rsp, "action"), "reset"));
> > +    qobject_unref(rsp);
> >      qtest_qmp_eventwait(qts, "RESET");
> >      qtest_quit(qts);
> >
> > --
> > 2.23.0
> >
>

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