[libvirt] PATCH: Switch all remaining code to memory alloc APIs

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> This patch switches all remaining code over to use the memory allocation
> APIs, with exception of virsh which is going to be slightly more complex
>
> It was mostly a straight conversion - there were only a few places which 
> weren't checking for failure corecttly - the most notable being sexpr.c.
...
Nice work (but tedious to review!).
I went through the whole thing this time.
Comments below.

> diff -r ff6b92c70738 src/conf.c
...
> @@ -897,15 +897,16 @@
>  
>      fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR );
>      if (fd < 0) {
> +        char *tmp = virBufferContentAndReset(&buf);
>          virConfError(NULL, VIR_ERR_WRITE_FAILED, _("failed to open file"), 0);
> -        free(virBufferContentAndReset(&buf));
> +        VIR_FREE(tmp);
>          return -1;
>      }
...

> diff -r ff6b92c70738 src/remote_internal.c
> --- a/src/remote_internal.c	Fri May 30 10:36:42 2008 -0400
> +++ b/src/remote_internal.c	Fri May 30 10:55:44 2008 -0400
...
> @@ -3749,14 +3744,13 @@
>      for (ninteract = 0 ; interact[ninteract].id != 0 ; ninteract++)
>          ; /* empty */
>  
> -    *cred = calloc(ninteract, sizeof(*cred));
> -    if (!*cred)
> +    if (VIR_ALLOC_N(*cred, ninteract) < 0)
>          return -1;

Cool!  You fixed a subtle bug right there.
The old code should have read like this:

       *cred = calloc(ninteract, sizeof(**cred));

Looks like it would have caused heap corruption, since sizeof(*cred)
is smaller than sizeof(**cred).

...
> diff -r ff6b92c70738 src/storage_backend_logical.c
> --- a/src/storage_backend_logical.c	Fri May 30 10:36:42 2008 -0400
> +++ b/src/storage_backend_logical.c	Fri May 30 10:55:44 2008 -0400
...
> @@ -266,7 +264,7 @@
>      memset(zeros, 0, sizeof(zeros));
>  
>      /* XXX multiple pvs */
> -    if ((vgargv = malloc(sizeof(char*) * (1))) == NULL) {
> +    if (VIR_ALLOC_N(vgargv, 1) < 0) {
>          virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("command line"));

That can be just

    if (VIR_ALLOC(vgargv) < 0) {

...
> diff -r ff6b92c70738 src/xen_unified.c
> --- a/src/xen_unified.c	Fri May 30 10:36:42 2008 -0400
> +++ b/src/xen_unified.c	Fri May 30 10:55:44 2008 -0400
> @@ -40,6 +40,7 @@
>  #include "xm_internal.h"
>  #include "xml.h"
>  #include "util.h"
> +#include "memory.h"
>  
>  #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt,__VA_ARGS__)
>  #define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
> @@ -172,15 +173,12 @@
>      if (xenUnifiedNodeGetInfo(dom->conn, &nodeinfo) < 0)
>          return(NULL);
>  
> -    cpulist = calloc(nb_cpu, sizeof(*cpulist));
> -    if (cpulist == NULL)
> +    if (VIR_ALLOC_N(cpulist, nb_cpu) < 0)
>          goto done;
> -    cpuinfo = malloc(sizeof(*cpuinfo) * nb_vcpu);
> -    if (cpuinfo == NULL)
> +    if (VIR_ALLOC_N(cpuinfo, nb_vcpu) < 0)
>          goto done;
>      cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo));
> -    cpumap = (unsigned char *) calloc(nb_vcpu, cpumaplen);
> -    if (cpumap == NULL)
> +    if (VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) < 0)
>          goto done;

At first I thought it didn't matter that the product wasn't
checked for overflow, but then I spent a couple minutes trying
to find if/where nb_vcpu was guaranteed to be small enough
that we don't have to worry.  There may well be code to ensure
that, but if so, it's too far from this point of use for my taste,
so I think it's best to add an explicit overflow check here, i.e.,

       if (xalloc_oversized(nb_vcpu, cpumaplen) ||
           VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) < 0)
           goto done;

...