[Libguestfs] [PATCH] daemon: initialize memory when handling DeviceList params

[Richard W.M. Jones]

On Fri, Aug 08, 2014 at 02:07:41PM +0200, Pino Toscano wrote:
> When dealing with DeviceList parameters, the generator produces code
> similar to the following:
> 
>   CLEANUP_FREE_STRING_LIST char **devices = NULL;
>   [...]
>   devices = malloc (sizeof (char *) * (args.devices.devices_len+1));
>   {
>     size_t i;
>     for (i = 0; i < args.devices.devices_len; ++i)
>       RESOLVE_DEVICE (args.devices.devices_val[i], devices[i],
>                       , goto done);
>     devices[i] = NULL;
>   }
> 
> The block hidden within the RESOLVE_DEVICE macro is supposed to
> assign something to devices[i]; on the other hand, the code in
> RESOLVE_DEVICE can cause to just end (with an error) the current RPC,
> which would cause the cleanup of the "devices" array... whose members
> from the i-th to the (args.devices.devices_len-1)-th would be garbage
> pointers, causing random memory to be free'd (and thus crashing the
> daemon).
> 
> Avoid the access to garbage memory just by having a cleaned "devices"
> array, so there will be always a NULL element after the initialized
> members.
> 
> Add a test for vgcreate which passes a wrong device path causing the
> situation above, to test that vgcreate would fail gracefully.
> ---
>  generator/actions.ml | 11 ++++++++++-
>  generator/daemon.ml  |  2 +-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 48213c5..9570d9b 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -3929,7 +3929,16 @@ as C</dev/sda1>." };
>           ["vgcreate"; "VG1"; "/dev/sda1 /dev/sda2"];
>           ["vgcreate"; "VG2"; "/dev/sda3"];
>           ["vgs"]],
> -        "is_string_list (ret, 2, \"VG1\", \"VG2\")"), []
> +        "is_string_list (ret, 2, \"VG1\", \"VG2\")"), [];
> +      InitEmpty, Always, TestLastFail (
> +        [["part_init"; "/dev/sda"; "mbr"];
> +         ["part_add"; "/dev/sda"; "p"; "64"; "204799"];
> +         ["part_add"; "/dev/sda"; "p"; "204800"; "409599"];
> +         ["part_add"; "/dev/sda"; "p"; "409600"; "-64"];
> +         ["pvcreate"; "/dev/sda1"];
> +         ["pvcreate"; "/dev/sda2"];
> +         ["pvcreate"; "/dev/sda3"];
> +         ["vgcreate"; "VG1"; "/foo/bar /dev/sda2"]]), [];
>      ];
>      shortdesc = "create an LVM volume group";
>      longdesc = "\
> diff --git a/generator/daemon.ml b/generator/daemon.ml
> index 951729c..06d49ef 100644
> --- a/generator/daemon.ml
> +++ b/generator/daemon.ml
> @@ -360,7 +360,7 @@ cleanup_free_mountable (mountable_t *mountable)
>              pr "  /* Copy the string list and apply device name translation\n";
>              pr "   * to each one.\n";
>              pr "   */\n";
> -            pr "  %s = malloc (sizeof (char *) * (args.%s.%s_len+1));\n" n n n;
> +            pr "  %s = calloc (args.%s.%s_len+1, sizeof (char *));\n" n n n;
>              pr "  {\n";
>              pr "    size_t i;\n";
>              pr "    for (i = 0; i < args.%s.%s_len; ++i)\n" n n;
> -- 
> 1.9.3

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html