Patchwork [v2] severe memory leak caused by broken palette_destroy() function

login
register
mail settings
Submitter Ulrich Obergfell
Date March 25, 2011, 8:45 a.m.
Message ID <963102742.505036.1301042754895.JavaMail.root@zmail07.collab.prod.int.phx2.redhat.com>
Download mbox | patch
Permalink /patch/88349/
State New
Headers show

Comments

Ulrich Obergfell - March 25, 2011, 8:45 a.m.
This is version 2 of the patch that I originally posted in:

http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg02063.html

[Sorry, I missed to include the keyword 'PATCH' in the subject
 of the original post.]

The following commit breaks the code of the function palette_destroy().

http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commit;h=e31e3694afef58ba191cbcc6875ec243e5971268

The broken code causes a severe memory leak of 'VncPalette' structures
because it never frees anything:

     70 void palette_destroy(VncPalette *palette)
     71 {
     72     if (palette == NULL) {
     73         qemu_free(palette);
     74     }
     75 }

Version 2 of the patch calls qemu_free() unconditionally.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
Juan Quintela - March 25, 2011, 9:28 a.m.
Ulrich Obergfell <uobergfe@redhat.com> wrote:
> This is version 2 of the patch that I originally posted in:
>
> http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg02063.html
>
> [Sorry, I missed to include the keyword 'PATCH' in the subject
>  of the original post.]
>
> The following commit breaks the code of the function palette_destroy().
>
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commit;h=e31e3694afef58ba191cbcc6875ec243e5971268
>
> The broken code causes a severe memory leak of 'VncPalette' structures
> because it never frees anything:
>
>      70 void palette_destroy(VncPalette *palette)
>      71 {
>      72     if (palette == NULL) {
>      73         qemu_free(palette);
>      74     }
>      75 }
>
> Version 2 of the patch calls qemu_free() unconditionally.
>
> Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>


Ouchhhhhhhhhhhhhhh

Reviewed-by: Juan Quintela <quintela@redhat.com>

A new reason to never ever test if pointer is != NULL before calling
free.

Good catch.
Stefan Hajnoczi - March 25, 2011, 9:31 a.m.
On Fri, Mar 25, 2011 at 8:45 AM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
>
> This is version 2 of the patch that I originally posted in:
>
> http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg02063.html
>
> [Sorry, I missed to include the keyword 'PATCH' in the subject
>  of the original post.]
>
> The following commit breaks the code of the function palette_destroy().
>
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commit;h=e31e3694afef58ba191cbcc6875ec243e5971268
>
> The broken code causes a severe memory leak of 'VncPalette' structures
> because it never frees anything:
>
>     70 void palette_destroy(VncPalette *palette)
>     71 {
>     72     if (palette == NULL) {
>     73         qemu_free(palette);
>     74     }
>     75 }
>
> Version 2 of the patch calls qemu_free() unconditionally.
>
> Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>

Looks good.

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Anthony Liguori - March 25, 2011, 12:36 p.m.
On 03/25/2011 04:31 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 25, 2011 at 8:45 AM, Ulrich Obergfell<uobergfe@redhat.com>  wrote:
>> This is version 2 of the patch that I originally posted in:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg02063.html
>>
>> [Sorry, I missed to include the keyword 'PATCH' in the subject
>>   of the original post.]
>>
>> The following commit breaks the code of the function palette_destroy().
>>
>> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commit;h=e31e3694afef58ba191cbcc6875ec243e5971268
>>
>> The broken code causes a severe memory leak of 'VncPalette' structures
>> because it never frees anything:
>>
>>      70 void palette_destroy(VncPalette *palette)
>>      71 {
>>      72     if (palette == NULL) {
>>      73         qemu_free(palette);
>>      74     }
>>      75 }
>>
>> Version 2 of the patch calls qemu_free() unconditionally.
>>
>> Signed-off-by: Ulrich Obergfell<uobergfe@redhat.com>
> Looks good.

Applied.  Thanks.

Regards,

Anthony Liguori

> Reviewed-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>

Patch

diff -up ./ui/vnc-palette.c.orig0 ./ui/vnc-palette.c
--- ./ui/vnc-palette.c.orig0	2011-03-15 03:53:22.000000000 +0100
+++ ./ui/vnc-palette.c	2011-03-21 20:19:02.736948725 +0100
@@ -69,9 +69,7 @@  void palette_init(VncPalette *palette, s
 
 void palette_destroy(VncPalette *palette)
 {
-    if (palette == NULL) {
-        qemu_free(palette);
-    }
+    qemu_free(palette);
 }
 
 int palette_put(VncPalette *palette, uint32_t color)