Re: [Xen-devel] [PATCH] libxenlight: fix heap overflow when domid_to_name returns NULL

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2011-03-03 at 23:29 +0000, Eamon Walsh wrote:
> The function flexarray_vappend() will stop at the first NULL
> argument.  In libxl_device_vfb_add(), this has been observed
> to result in keys being added to the backend array without
> associated values in cases where the value can be NULL.

If these values are NULL should we be writing them at all? e.g. for:
        flexarray_vappend(back, foo, bar);
where bar may be NULL shouldn't it become:
        if (bar) 
                flexarray_vappend(back, foo, bar);
or perhaps:
        flexarray_vappend(back, foo, bar ? bar : "");
?

> This causes libxl__xs_kvs_of_flexarray(), which expects an even
> number of array elements, to write past the end of the array.

Sounds like libxl__xs_kvs_of_flexarray could also be made more robust
here...

Ian.

> 
> Fix by manually appending two values.
> 
> Signed-off-by: Eamon Walsh <ewalsh@xxxxxxxxxxxxx>
> ---
> 
> diff -r c5d121fd35c0 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Mon Feb 28 16:55:20 2011 +0000
> +++ b/tools/libxl/libxl.c     Thu Mar 03 18:32:10 2011 -0500
> @@ -1890,9 +1890,11 @@
>      flexarray_vappend(back, "frontend-id", libxl__sprintf(&gc, "%d", 
> vfb->domid), NULL);
>      flexarray_vappend(back, "online", "1", NULL);
>      flexarray_vappend(back, "state", libxl__sprintf(&gc, "%d", 1), NULL);
> -    flexarray_vappend(back, "domain", libxl__domid_to_name(&gc, domid), 
> NULL);
> +    flexarray_append(back, "domain");
> +    flexarray_append(back, libxl__domid_to_name(&gc, domid));
>      flexarray_vappend(back, "vnc", libxl__sprintf(&gc, "%d", vfb->vnc), 
> NULL);
> -    flexarray_vappend(back, "vnclisten", vfb->vnclisten, NULL);
> +    flexarray_append(back, "vnclisten");
> +    flexarray_append(back, vfb->vnclisten);
>      flexarray_append(back, "vncpasswd");
>      flexarray_append(back, vfb->vncpasswd);
>      flexarray_vappend(back, "vncdisplay", libxl__sprintf(&gc, "%d", 
> vfb->vncdisplay), NULL);
> 
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel