[Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Stefan Weil]

cppcheck report:
  gdbstub.c:1781: error: Memory leak: s

Rearranging of the code avoids the leak.

The patch also slightly cleans the g_malloc0 statement which was
touched by that change (no type cast, easier code review).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 gdbstub.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 4009058..34746f2 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1768,12 +1768,6 @@ void gdb_register_coprocessor(CPUState * env,
     GDBRegisterState **p;
     static int last_reg = NUM_CORE_REGS;
 
-    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
-    s->base_reg = last_reg;
-    s->num_regs = num_regs;
-    s->get_reg = get_reg;
-    s->set_reg = set_reg;
-    s->xml = xml;
     p = &env->gdb_regs;
     while (*p) {
         /* Check for duplicates.  */
@@ -1781,6 +1775,14 @@ void gdb_register_coprocessor(CPUState * env,
             return;
         p = &(*p)->next;
     }
+
+    s = g_malloc0(sizeof(*s));
+    s->base_reg = last_reg;
+    s->num_regs = num_regs;
+    s->get_reg = get_reg;
+    s->set_reg = set_reg;
+    s->xml = xml;
+
     /* Add to end of list.  */
     last_reg += num_regs;
     *p = s;
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Stuart Brady]

On Mon, Oct 17, 2011 at 10:01:25PM +0200, Stefan Weil wrote:
> 
> The patch also slightly cleans the g_malloc0 statement which was
> touched by that change (no type cast, easier code review).
[...]
> -    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
[...]
> +    s = g_malloc0(sizeof(*s));

Is this the preferred style, or should we be using:

    s = g_new0(GDBRegisterState, 1);

We currently seem to have a mix of the two styles.

I kinda prefer using g_new() (or g_new0()) since provided the 'count'
parameter is correct, it can't really be wrong, whereas using sizeof()
has the potential to be buggy, even if it is still trivial to check
that the code is okay.

I guess I feel g_malloc() would be best reserved for those cases where
we're doing something special which might need a little extra care...

BTW, I've just been experimenting with Coccinelle and produced the
following semantic patch:

   @@ type T; @@
   -(T *)g_malloc(sizeof(T))
   +g_new(T, 1)

   @@ type T; @@
   -(T *)g_malloc0(sizeof(T))
   +g_new0(T, 1)

   @@ type T; expression E; @@
   -(T *)g_malloc(sizeof(T) * E)
   +g_new(T, E)

   @@ type T; expression E; @@
   -(T *)g_malloc0(sizeof(T) * E)
   +g_new0(T, E)

I couldn't get the following working:

   // THIS DOES NOT WORK:
   @@ type T; identifier I; @@
   -T I = g_malloc(sizeof(*I))
   +T I = g_new(T, 1)

I expect this won't work either:

   // THIS PROBABLY DOES NOT WORK EITHER:
   @@ type T; identifier I; @@
   -T I = g_new(T, 1)
   +T I = g_malloc(sizeof(*I))

Cheers,
-- 
Stuart

Re: [Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Blue Swirl]

On Tue, Oct 18, 2011 at 1:13 AM, Stuart Brady <sdb@zubnet.me.uk> wrote:
> On Mon, Oct 17, 2011 at 10:01:25PM +0200, Stefan Weil wrote:
>>
>> The patch also slightly cleans the g_malloc0 statement which was
>> touched by that change (no type cast, easier code review).
> [...]
>> -    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
> [...]
>> +    s = g_malloc0(sizeof(*s));
>
> Is this the preferred style, or should we be using:
>
>    s = g_new0(GDBRegisterState, 1);
>
> We currently seem to have a mix of the two styles.
>
> I kinda prefer using g_new() (or g_new0()) since provided the 'count'
> parameter is correct, it can't really be wrong, whereas using sizeof()
> has the potential to be buggy, even if it is still trivial to check
> that the code is okay.
>
> I guess I feel g_malloc() would be best reserved for those cases where
> we're doing something special which might need a little extra care...
>
> BTW, I've just been experimenting with Coccinelle and produced the
> following semantic patch:
>
>   @@ type T; @@
>   -(T *)g_malloc(sizeof(T))
>   +g_new(T, 1)
>
>   @@ type T; @@
>   -(T *)g_malloc0(sizeof(T))
>   +g_new0(T, 1)
>
>   @@ type T; expression E; @@
>   -(T *)g_malloc(sizeof(T) * E)
>   +g_new(T, E)
>
>   @@ type T; expression E; @@
>   -(T *)g_malloc0(sizeof(T) * E)
>   +g_new0(T, E)

Cool. Please include the spatch with the commit message.

> I couldn't get the following working:
>
>   // THIS DOES NOT WORK:
>   @@ type T; identifier I; @@
>   -T I = g_malloc(sizeof(*I))
>   +T I = g_new(T, 1)
>
> I expect this won't work either:
>
>   // THIS PROBABLY DOES NOT WORK EITHER:
>   @@ type T; identifier I; @@
>   -T I = g_new(T, 1)
>   +T I = g_malloc(sizeof(*I))

IIRC I had also problems with identifiers, I could not get checking
identifiers against CODING_STYLE to work.

Re: [Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Stuart Brady]

On Tue, Oct 18, 2011 at 06:18:11PM +0000, Blue Swirl wrote:

> Cool. Please include the spatch with the commit message.

Thanks, will do!

FWIW, it results in:

 51 files changed, 99 insertions(+), 136 deletions(-)

I wonder if that needs splitting up at all?

> IIRC I had also problems with identifiers, I could not get checking
> identifiers against CODING_STYLE to work.

FWIW, I got there in the end:

   @@ type T; T *E; @@
   -E = g_malloc(sizeof(*E))
   +E = g_new(T, 1)

   @@ type T; T *E; @@
   -E = g_malloc0(sizeof(*E))
   +E = g_new0(T, 1)

   @@ type T; T *E; expression N; @@
   -E = g_malloc(sizeof(*E) * N)
   +E = g_new(T, N)

   @@ type T; T *E; expression N; @@
   -E = g_malloc0(sizeof(*E) * N)
   +E = g_new0(T, N)

   @@ type T; T *E; @@
   -E = g_malloc(sizeof(T))
   +E = g_new(T, 1)

   @@ type T; T *E; @@
   -E = g_malloc0(sizeof(T))
   +E = g_new0(T, 1)

   @@ type T; T *E; expression N; @@
   -E = g_malloc(sizeof(T) * N)
   +E = g_new(T, N)

   @@ type T; T *E; expression N; @@
   -E = g_malloc0(sizeof(T) * N)
   +E = g_new0(T, N)

With this added, I get:

 246 files changed, 514 insertions(+), 557 deletions(-)

That includes the original 99 insertions and 136 deletions.

I think I should at least submit the patch to convert cases involving
casts as the first part of the patch series, and convert the sizeof(*E)
and sizeof(T) cases as the second and third parts.  Sound okay?

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Stuart Brady]

On Wed, Oct 19, 2011 at 01:59:04AM +0100, Stuart Brady wrote:
> On Tue, Oct 18, 2011 at 06:18:11PM +0000, Blue Swirl wrote:
> 
> > Cool. Please include the spatch with the commit message.
> 
> Thanks, will do!

Okay, submitted.

This is the first time I've used git send-email, so let me know if I've
missed anything.

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH] gdbstub: Fix memory leak

[Blue Swirl]

On Thu, Oct 20, 2011 at 08:10, Stuart Brady <sdb@zubnet.me.uk> wrote:
> On Wed, Oct 19, 2011 at 01:59:04AM +0100, Stuart Brady wrote:
>> On Tue, Oct 18, 2011 at 06:18:11PM +0000, Blue Swirl wrote:
>>
>> > Cool. Please include the spatch with the commit message.
>>
>> Thanks, will do!
>
> Okay, submitted.
>
> This is the first time I've used git send-email, so let me know if I've
> missed anything.

The patches are OK, though when sending multiple patches, a cover
letter (--cover-letter) is appreciated.

[Qemu-devel] [PATCH v2] gdbstub: Fix memory leak

[Stefan Weil]

cppcheck report:
  gdbstub.c:1781: error: Memory leak: s

Rearranging of the code avoids the leak.

v2:
Replace the g_malloc0() by g_new0() (suggested by Stuart Brady).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 gdbstub.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 4009058..8bf7167 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1768,12 +1768,6 @@ void gdb_register_coprocessor(CPUState * env,
     GDBRegisterState **p;
     static int last_reg = NUM_CORE_REGS;
 
-    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
-    s->base_reg = last_reg;
-    s->num_regs = num_regs;
-    s->get_reg = get_reg;
-    s->set_reg = set_reg;
-    s->xml = xml;
     p = &env->gdb_regs;
     while (*p) {
         /* Check for duplicates.  */
@@ -1781,6 +1775,14 @@ void gdb_register_coprocessor(CPUState * env,
             return;
         p = &(*p)->next;
     }
+
+    s = g_new0(GDBRegisterState, 1);
+    s->base_reg = last_reg;
+    s->num_regs = num_regs;
+    s->get_reg = get_reg;
+    s->set_reg = set_reg;
+    s->xml = xml;
+
     /* Add to end of list.  */
     last_reg += num_regs;
     *p = s;
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2] gdbstub: Fix memory leak

[Stefan Weil]

Am 18.10.2011 22:25, schrieb Stefan Weil:
> cppcheck report:
>    gdbstub.c:1781: error: Memory leak: s
>
> Rearranging of the code avoids the leak.
>
> v2:
> Replace the g_malloc0() by g_new0() (suggested by Stuart Brady).
>
> Signed-off-by: Stefan Weil<sw@weilnetz.de>
> ---
>   gdbstub.c |   14 ++++++++------
>   1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 4009058..8bf7167 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1768,12 +1768,6 @@ void gdb_register_coprocessor(CPUState * env,
>       GDBRegisterState **p;
>       static int last_reg = NUM_CORE_REGS;
>
> -    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
> -    s->base_reg = last_reg;
> -    s->num_regs = num_regs;
> -    s->get_reg = get_reg;
> -    s->set_reg = set_reg;
> -    s->xml = xml;
>       p =&env->gdb_regs;
>       while (*p) {
>           /* Check for duplicates.  */
> @@ -1781,6 +1775,14 @@ void gdb_register_coprocessor(CPUState * env,
>               return;
>           p =&(*p)->next;
>       }
> +
> +    s = g_new0(GDBRegisterState, 1);
> +    s->base_reg = last_reg;
> +    s->num_regs = num_regs;
> +    s->get_reg = get_reg;
> +    s->set_reg = set_reg;
> +    s->xml = xml;
> +
>       /* Add to end of list.  */
>       last_reg += num_regs;
>       *p = s;
>    


Ping? This patch is still missing for QEMU 1.0.

Kind regards,

Stefan Weil

Re: [Qemu-devel] [PATCH v2] gdbstub: Fix memory leak

[Blue Swirl]

Thanks, applied.

On Fri, Nov 11, 2011 at 21:09, Stefan Weil <sw@weilnetz.de> wrote:
> Am 18.10.2011 22:25, schrieb Stefan Weil:
>>
>> cppcheck report:
>>   gdbstub.c:1781: error: Memory leak: s
>>
>> Rearranging of the code avoids the leak.
>>
>> v2:
>> Replace the g_malloc0() by g_new0() (suggested by Stuart Brady).
>>
>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>> ---
>>  gdbstub.c |   14 ++++++++------
>>  1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 4009058..8bf7167 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1768,12 +1768,6 @@ void gdb_register_coprocessor(CPUState * env,
>>      GDBRegisterState **p;
>>      static int last_reg = NUM_CORE_REGS;
>>
>> -    s = (GDBRegisterState *)g_malloc0(sizeof(GDBRegisterState));
>> -    s->base_reg = last_reg;
>> -    s->num_regs = num_regs;
>> -    s->get_reg = get_reg;
>> -    s->set_reg = set_reg;
>> -    s->xml = xml;
>>      p =&env->gdb_regs;
>>      while (*p) {
>>          /* Check for duplicates.  */
>> @@ -1781,6 +1775,14 @@ void gdb_register_coprocessor(CPUState * env,
>>              return;
>>          p =&(*p)->next;
>>      }
>> +
>> +    s = g_new0(GDBRegisterState, 1);
>> +    s->base_reg = last_reg;
>> +    s->num_regs = num_regs;
>> +    s->get_reg = get_reg;
>> +    s->set_reg = set_reg;
>> +    s->xml = xml;
>> +
>>      /* Add to end of list.  */
>>      last_reg += num_regs;
>>      *p = s;
>>
>
>
> Ping? This patch is still missing for QEMU 1.0.
>
> Kind regards,
>
> Stefan Weil
>
>
>