Patchwork [v2] loader: don't call realloc(non_null, 0) when no symbols are present

login
register
mail settings
Submitter Aurelien Jarno
Date Dec. 28, 2009, 8:20 p.m.
Message ID <20091228202020.GB4139@volta.aurel32.net>
Download mbox | patch
Permalink /patch/41861/
State New
Headers show

Comments

Aurelien Jarno - Dec. 28, 2009, 8:20 p.m.
According to C99, realloc(non_null, 0) != free(non_null), that's why
it is forbidden in QEMU.

When there are no symbols, nsyms equals to 0. Free the syms structure
and set it to NULL instead of reallocating it with a size of 0.

This fixes -kernel with stripped kernels.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/elf_ops.h |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Michael S. Tsirkin - Dec. 29, 2009, 3:36 p.m.
On Mon, Dec 28, 2009 at 09:20:20PM +0100, Aurelien Jarno wrote:
> According to C99, realloc(non_null, 0) != free(non_null), that's why
> it is forbidden in QEMU.
> 
> When there are no symbols, nsyms equals to 0. Free the syms structure
> and set it to NULL instead of reallocating it with a size of 0.
> 
> This fixes -kernel with stripped kernels.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

I didn't know, you live and learn. FWIW
Acked-by: Michael S. Tsirkin <mst@redhat.com>

BTW, which systems implement this according to C99?  glibc seems to do
free(non_null) on fedora 11.

> ---
>  hw/elf_ops.h |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/elf_ops.h b/hw/elf_ops.h
> index 6093dea..d0811ca 100644
> --- a/hw/elf_ops.h
> +++ b/hw/elf_ops.h
> @@ -149,9 +149,14 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
>          }
>          i++;
>      }
> -    syms = qemu_realloc(syms, nsyms * sizeof(*syms));
> +    if (nsyms) {
> +        syms = qemu_realloc(syms, nsyms * sizeof(*syms));
>  
> -    qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
> +        qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
> +    } else {
> +        free(syms);
> +        syms = NULL;
> +    }
>  
>      /* String table */
>      if (symtab->sh_link >= ehdr->e_shnum)
> -- 
> 1.6.5.3
> 
>
Aurelien Jarno - Dec. 29, 2009, 4:26 p.m.
On Tue, Dec 29, 2009 at 05:36:40PM +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 28, 2009 at 09:20:20PM +0100, Aurelien Jarno wrote:
> > According to C99, realloc(non_null, 0) != free(non_null), that's why
> > it is forbidden in QEMU.
> > 
> > When there are no symbols, nsyms equals to 0. Free the syms structure
> > and set it to NULL instead of reallocating it with a size of 0.
> > 
> > This fixes -kernel with stripped kernels.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> I didn't know, you live and learn. FWIW

Can you please stop being annoying about that? I submit non-trivial 
patches like this one before committing them. This is nothing new.
Michael S. Tsirkin - Dec. 29, 2009, 4:34 p.m.
On Tue, Dec 29, 2009 at 05:26:50PM +0100, Aurelien Jarno wrote:
> On Tue, Dec 29, 2009 at 05:36:40PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Dec 28, 2009 at 09:20:20PM +0100, Aurelien Jarno wrote:
> > > According to C99, realloc(non_null, 0) != free(non_null), that's why
> > > it is forbidden in QEMU.
> > > 
> > > When there are no symbols, nsyms equals to 0. Free the syms structure
> > > and set it to NULL instead of reallocating it with a size of 0.
> > > 
> > > This fixes -kernel with stripped kernels.
> > > 
> > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > 
> > I didn't know, you live and learn. FWIW
> 
> Can you please stop being annoying about that? I submit non-trivial 
> patches like this one before committing them. This is nothing new.

Oh, this is a misunderstanding, sorry.  I really meant that I lived
another day and learned about realloc(p, 0) being implementation
dependend. That is all.

> -- 
> Aurelien Jarno	                        GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
Jamie Lokier - Dec. 29, 2009, 4:40 p.m.
Aurelien Jarno wrote:
> On Tue, Dec 29, 2009 at 05:36:40PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Dec 28, 2009 at 09:20:20PM +0100, Aurelien Jarno wrote:
> > > According to C99, realloc(non_null, 0) != free(non_null), that's why
> > > it is forbidden in QEMU.
> > > 
> > > When there are no symbols, nsyms equals to 0. Free the syms structure
> > > and set it to NULL instead of reallocating it with a size of 0.
> > > 
> > > This fixes -kernel with stripped kernels.
> > > 
> > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > 
> > I didn't know, you live and learn. FWIW
> 
> Can you please stop being annoying about that?

What was the annoyance?  I don't see what it is and would like to
avoid being annoying.

Thanks,
-- Jamie
Aurelien Jarno - Dec. 29, 2009, 5:25 p.m.
On Tue, Dec 29, 2009 at 04:40:33PM +0000, Jamie Lokier wrote:
> Aurelien Jarno wrote:
> > On Tue, Dec 29, 2009 at 05:36:40PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Dec 28, 2009 at 09:20:20PM +0100, Aurelien Jarno wrote:
> > > > According to C99, realloc(non_null, 0) != free(non_null), that's why
> > > > it is forbidden in QEMU.
> > > > 
> > > > When there are no symbols, nsyms equals to 0. Free the syms structure
> > > > and set it to NULL instead of reallocating it with a size of 0.
> > > > 
> > > > This fixes -kernel with stripped kernels.
> > > > 
> > > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > > 
> > > I didn't know, you live and learn. FWIW
> > 
> > Can you please stop being annoying about that?
> 
> What was the annoyance?  I don't see what it is and would like to
> avoid being annoying.

A misunderstanding between Michael and me. Sorry about that.

Patch

diff --git a/hw/elf_ops.h b/hw/elf_ops.h
index 6093dea..d0811ca 100644
--- a/hw/elf_ops.h
+++ b/hw/elf_ops.h
@@ -149,9 +149,14 @@  static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
         }
         i++;
     }
-    syms = qemu_realloc(syms, nsyms * sizeof(*syms));
+    if (nsyms) {
+        syms = qemu_realloc(syms, nsyms * sizeof(*syms));
 
-    qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
+        qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
+    } else {
+        free(syms);
+        syms = NULL;
+    }
 
     /* String table */
     if (symtab->sh_link >= ehdr->e_shnum)