Re: xl network-attach SEGV in 4.2 and 4.1

["Roger Pau Monné" <roger.pau@citrix.com>]

On 18/04/13 18:01, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] xl network-attach SEGV in 4.2 and 4.1"):
>> On Wed, 2013-04-17 at 03:49 +0100, Marek Marczykowski wrote:
>>> Hi all,
>>> When "device/vif" directory exists but is empty l!=NULL, but nb==0, so
>>> l[nb-1] is invalid. Add missing check.
> 
> Thanks for pointing this out.  The root cause is the expectation that
> libxl__xs_directory won't return a non-null list but set *nb to 0.
> 
> I found some occurrences of this bug in (xen-unstable) staging.
> See patch below, which should probably go into 4.1 and 4.2.
> 
>>> Signed-Off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
>>>
>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>>> index 5783cd2..9e06a7d 100644
>>> --- a/tools/libxl/libxl.c
>>> +++ b/tools/libxl/libxl.c
>>> @@ -2569,7 +2569,8 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t
>>> domid,
>>>              goto out_free;
>>>          }
>>>          if (!(l = libxl__xs_directory(gc, XBT_NULL,
>>> -                                     libxl__sprintf(gc, "%s/device/vif",
>>> dompath), &nb))) {
>>> +                                     libxl__sprintf(gc, "%s/device/vif",
>>> dompath), &nb)) ||
>>> +                nb == 0) {
> 
> I think this is the right way to fix this in the stable trees.  I
> looked at the backport of "5420f2650 libxl: Set vfb and vkb devid if
> not done so by the caller" and it's nontrivial, so although it's a
> bugfix I'd rather leave it.
> 
> Ian.
> 
> 
> commit d1f52920ee96c91665813fb875c0c76e5eb7a312
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Thu Apr 18 16:27:46 2013 +0100
> 
>     libxl: Avoid realloc(,0) when libxl__xs_directory returns empty list
>     
>     If the named path is a leaf node, libxl__xs_directory can succeed,
>     returning non-null, but set *nb to 0.
>     
>     In three places in libxl this may result in a zero size argument being
>     passed to malloc() or realloc(), which is not adviseable.
>     
>     Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 0f936c0..1c04d37 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1840,7 +1840,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n
>  
>      fe_path = libxl__sprintf(gc, "%s/device/vtpm", libxl__xs_get_dompath(gc, domid));
>      dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
> -    if(dir) {
> +    if (dir && ndirs) {
>         vtpms = malloc(sizeof(*vtpms) * ndirs);
>         libxl_device_vtpm* vtpm;
>         libxl_device_vtpm* end = vtpms + ndirs;
> @@ -2314,7 +2314,7 @@ static int libxl__append_disk_list_of_type(libxl__gc *gc,
>      be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                               libxl__xs_get_dompath(gc, 0), type, domid);
>      dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
> -    if (dir) {
> +    if (dir && n) {
>          libxl_device_disk *tmp;
>          tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
>          if (tmp == NULL)
> @@ -3006,7 +3006,7 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>      be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                               libxl__xs_get_dompath(gc, 0), type, domid);
>      dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
> -    if (dir) {
> +    if (dir && n) {
>          libxl_device_nic *tmp;
>          tmp = realloc(*nics, sizeof (libxl_device_nic) * (*nnics + n));
>          if (tmp == NULL)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>