[Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[Blue Swirl]

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

Hi,

This patch adds simple checks for qemu_malloc/free/realloc. With the
check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
system emulators are fine.

[-- Attachment #2: qemu_malloc_debug.diff --]
[-- Type: plain/text, Size: 1721 bytes --]

Re: [Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[malc]

On Wed, 15 Apr 2009, Blue Swirl wrote:

> Hi,
> 
> This patch adds simple checks for qemu_malloc/free/realloc. With the
> check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
> system emulators are fine.

kvm_has_msr_star plain free is used on qemu_malloczed region. Perhaps
something similar is done in one of the devices that are only enabled
on i386.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[Kevin Wolf]

Am Mittwoch, 15. April 2009 20:23 schrieb Blue Swirl:
> This patch adds simple checks for qemu_malloc/free/realloc. With the
> check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
> system emulators are fine.

Generally I like the idea of running this kind of tests occasionally. I'm 
wondering though if we really need additional code for it, or if using 
external tools like valgrind could do the same without additional code and 
with the extra bonus of actually telling you who the culprit was.

> Index: qemu/qemu-malloc.c
> ===================================================================
> --- qemu.orig/qemu-malloc.c     2009-04-15 17:16:44.000000000 +0000
> +++ qemu/qemu-malloc.c  2009-04-15 18:09:49.000000000 +0000
> @@ -24,6 +24,11 @@
>  #include "qemu-common.h"
>  #include <stdlib.h>
>  
> +#define DEBUG_MALLOC
> +
> +/* Canary must not break malloc alignment */
> +static const char canary[] = "QEMU_MALLOC1234";
> +
>  static void *oom_check(void *ptr)
>  {
>      if (ptr == NULL)
> @@ -38,20 +43,58 @@
>  
>  void qemu_free(void *ptr)
>  {
> +#ifdef DEBUG_MALLOC
> +    fprintf(stderr, "qemu_free: 0x%p\n", ptr);
> +    if (ptr) {
> +        ptr = (void *)((unsigned long)ptr - sizeof(canary));
> +        if (memcmp(ptr, canary, sizeof(canary)) != 0) {
> +            exit(0);

I'm used to the convention that an exit code of 0 means success. abort() might 
be the right thing here.

Kevin

Re: [Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[malc]

On Wed, 15 Apr 2009, Blue Swirl wrote:

> Hi,
> 
> This patch adds simple checks for qemu_malloc/free/realloc. With the
> check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
> system emulators are fine.
> 

The check for qemu_malloc result in qemu_realloc is not needed.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[Blue Swirl]

On 4/15/09, Kevin Wolf <mail@kevin-wolf.de> wrote:
> Am Mittwoch, 15. April 2009 20:23 schrieb Blue Swirl:
>
> > This patch adds simple checks for qemu_malloc/free/realloc. With the
>  > check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
>  > system emulators are fine.
>
>
> Generally I like the idea of running this kind of tests occasionally. I'm
>  wondering though if we really need additional code for it, or if using
>  external tools like valgrind could do the same without additional code and
>  with the extra bonus of actually telling you who the culprit was.

The check examines whether the memory allocated with qemu_malloc and
friends is freed with plain free (or malloc'ed memory freed with
qemu_free). We are not checking if the memory is corrupted that
Valgrind checks for. Maybe semantic patch stuff might be able to catch
this type of errors.

>  > Index: qemu/qemu-malloc.c
>  > ===================================================================
>  > --- qemu.orig/qemu-malloc.c     2009-04-15 17:16:44.000000000 +0000
>  > +++ qemu/qemu-malloc.c  2009-04-15 18:09:49.000000000 +0000
>  > @@ -24,6 +24,11 @@
>  >  #include "qemu-common.h"
>  >  #include <stdlib.h>
>  >
>  > +#define DEBUG_MALLOC
>  > +
>  > +/* Canary must not break malloc alignment */
>  > +static const char canary[] = "QEMU_MALLOC1234";
>  > +
>  >  static void *oom_check(void *ptr)
>  >  {
>  >      if (ptr == NULL)
>  > @@ -38,20 +43,58 @@
>  >
>  >  void qemu_free(void *ptr)
>  >  {
>  > +#ifdef DEBUG_MALLOC
>  > +    fprintf(stderr, "qemu_free: 0x%p\n", ptr);
>  > +    if (ptr) {
>  > +        ptr = (void *)((unsigned long)ptr - sizeof(canary));
>  > +        if (memcmp(ptr, canary, sizeof(canary)) != 0) {
>  > +            exit(0);
>
>  I'm used to the convention that an exit code of 0 means success. abort() might
>  be the right thing here.

Right.

Re: [Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[Blue Swirl]

On 4/15/09, malc <av1474@comtv.ru> wrote:
> On Wed, 15 Apr 2009, Blue Swirl wrote:
>
>  > Hi,
>  >
>  > This patch adds simple checks for qemu_malloc/free/realloc. With the
>  > check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
>  > system emulators are fine.
>
>
> kvm_has_msr_star plain free is used on qemu_malloczed region. Perhaps
>  something similar is done in one of the devices that are only enabled
>  on i386.

Actually the crash was caused by a bug in the checker.

Re: [Qemu-devel] [PATCH] [RFC] qemu_malloc dynamic checking

[malc]

On Thu, 16 Apr 2009, Blue Swirl wrote:

> On 4/15/09, malc <av1474@comtv.ru> wrote:
> > On Wed, 15 Apr 2009, Blue Swirl wrote:
> >
> >  > Hi,
> >  >
> >  > This patch adds simple checks for qemu_malloc/free/realloc. With the
> >  > check enabled, i386-softmmu crashes. Sparc32, Sparc64, PPC32 and PPC64
> >  > system emulators are fine.
> >
> >
> > kvm_has_msr_star plain free is used on qemu_malloczed region. Perhaps
> >  something similar is done in one of the devices that are only enabled
> >  on i386.
> 
> Actually the crash was caused by a bug in the checker.

memcpy in realloc?

-- 
mailto:av1474@comtv.ru