All of lore.kernel.org
 help / color / mirror / Atom feed
* xl network-attach SEGV in 4.2 and 4.1
@ 2013-04-17  2:49 Marek Marczykowski
  2013-04-17  8:43 ` Ian Campbell
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Marczykowski @ 2013-04-17  2:49 UTC (permalink / raw)
  To: xen-devel

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

4.2 and 4.1 suffers from SEGV during xl network-attach in
libxl__device_nic_add. In 4.3-unstable it is fixed by:
5420f2650 libxl: Set vfb and vkb devid if not done so by the caller

So either the patch need to be backported to 4.1 and 4.2, or fixed by this one:
- ------
libxl: Fix SEGV in network-attach

When "device/vif" directory exists but is empty l!=NULL, but nb==0, so
l[nb-1] is invalid. Add missing check.

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) {
             nic->devid = 0;
         } else {
             nic->devid = strtoul(l[nb - 1], NULL, 10) + 1;

- -- 
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iQEcBAEBAgAGBQJRbg3PAAoJENuP0xzK19cs0qQH/2mR6lUwj75tKCckFJfM7HPJ
6YEqDvdunmCXr7Wcf9p+Z6RRsnjie9ihDwxbaP4b5XYMLW59YbvZEb3tyrIADYJy
gGwE+8xn3Jx0NjiRCJF5O2nF3w9I5W9o5sUMbXporo/Tu4X0e3PJ6wb1JseZWsLL
mVl3bjwIChos6eKsAKOuHIPqFCASsKIGJibrkWva9LqQ4k/xgjTshCgmbAgEITEo
8fAZbTRL/SM91qX5a+2ynUf/0h5Sb/IvLyLwm86n1wsqLoJ261VMgAOsXxYGUTBM
Pr5Gvc+s5VdhA0iY4IYE8GuQ99MnaXO6T+JSenpfJ3L26LSMvHkImFLqI3ljhzE=
=FWNX
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: xl network-attach SEGV in 4.2 and 4.1
  2013-04-17  2:49 xl network-attach SEGV in 4.2 and 4.1 Marek Marczykowski
@ 2013-04-17  8:43 ` Ian Campbell
  2013-04-18 16:01   ` Ian Jackson
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-04-17  8:43 UTC (permalink / raw)
  To: Marek Marczykowski; +Cc: Ian Jackson, xen-devel

Copying the stable tools maintainer. Please always do so for backport
requests (see MAINTAINERS in the relevant tree to see who to copy).

On Wed, 2013-04-17 at 03:49 +0100, Marek Marczykowski wrote:
> Hi all,
> 
> 4.2 and 4.1 suffers from SEGV during xl network-attach in
> libxl__device_nic_add. In 4.3-unstable it is fixed by:
> 5420f2650 libxl: Set vfb and vkb devid if not done so by the caller
> 
> So either the patch need to be backported to 4.1 and 4.2,

On the face of it I can't see any problem with backporting that commit

>  or fixed by this one:
> ------
> libxl: Fix SEGV in network-attach
> 
> When "device/vif" directory exists but is empty l!=NULL, but nb==0, so
> l[nb-1] is invalid. Add missing check.
> 
> 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) {
>              nic->devid = 0;
>          } else {
>              nic->devid = strtoul(l[nb - 1], NULL, 10) + 1;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: xl network-attach SEGV in 4.2 and 4.1
  2013-04-17  8:43 ` Ian Campbell
@ 2013-04-18 16:01   ` Ian Jackson
  2013-04-18 16:46     ` xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages] Ian Jackson
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ian Jackson @ 2013-04-18 16:01 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Marek Marczykowski, xen-devel

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>

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)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages]
  2013-04-18 16:01   ` Ian Jackson
@ 2013-04-18 16:46     ` Ian Jackson
  2013-04-19  7:12       ` Jan Beulich
  2013-04-18 18:58     ` xl network-attach SEGV in 4.2 and 4.1 Roger Pau Monné
  2013-11-12 17:23     ` [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list Ian Jackson
  2 siblings, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2013-04-18 16:46 UTC (permalink / raw)
  To: Marek Marczykowski; +Cc: Ian Campbell, Jan Beulich, xen-devel

Marek Marczykowski writes ("[Xen-devel] xl network-attach SEGV in 4.2 and 4.1"):
> 4.2 and 4.1 suffers from SEGV during xl network-attach in
> libxl__device_nic_add. In 4.3-unstable it is fixed by:
> 5420f2650 libxl: Set vfb and vkb devid if not done so by the caller
> 
> So either the patch need to be backported to 4.1 and 4.2, or fixed by this one:
> - ------
> libxl: Fix SEGV in network-attach

I have applied this to 4.2 and 4.1.  I had to fix up a conflict in 4.1
and also I fixed a whitespace bug.  The patches as committed are below.

> When "device/vif" directory exists but is empty l!=NULL, but nb==0, so
> l[nb-1] is invalid. Add missing check.
...
>          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) {

Something in the arrangements you used to prepare this patch
linewrapped it _before you signed it_.

Ian Jackson writes ("Re: [Xen-devel] xl network-attach SEGV in 4.2 and 4.1"):
> 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

IWBNI someone would review this and ack it; I'd like it to go into
4.1, 4.2 and unstable.

Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: xl network-attach SEGV in 4.2 and 4.1
  2013-04-18 16:01   ` Ian Jackson
  2013-04-18 16:46     ` xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages] Ian Jackson
@ 2013-04-18 18:58     ` Roger Pau Monné
  2013-11-12 17:23     ` [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list Ian Jackson
  2 siblings, 0 replies; 11+ messages in thread
From: Roger Pau Monné @ 2013-04-18 18:58 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Marek Marczykowski

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages]
  2013-04-18 16:46     ` xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages] Ian Jackson
@ 2013-04-19  7:12       ` Jan Beulich
  2013-04-22  9:49         ` Ian Jackson
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2013-04-19  7:12 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Ian Campbell, Marek Marczykowski

>>> On 18.04.13 at 18:46, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Marek Marczykowski writes ("[Xen-devel] xl network-attach SEGV in 4.2 and 
> 4.1"):
>> 4.2 and 4.1 suffers from SEGV during xl network-attach in
>> libxl__device_nic_add. In 4.3-unstable it is fixed by:
>> 5420f2650 libxl: Set vfb and vkb devid if not done so by the caller
>> 
>> So either the patch need to be backported to 4.1 and 4.2, or fixed by this 
> one:
>> - ------
>> libxl: Fix SEGV in network-attach
> 
> I have applied this to 4.2 and 4.1.  I had to fix up a conflict in 4.1
> and also I fixed a whitespace bug.  The patches as committed are below.

This fixup on 4.1 went wrong - you dropped an & that's necessary.
I'll take the liberty to fix this right away.

Jan

>> When "device/vif" directory exists but is empty l!=NULL, but nb==0, so
>> l[nb-1] is invalid. Add missing check.
> ...
>>          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) {
> 
> Something in the arrangements you used to prepare this patch
> linewrapped it _before you signed it_.
> 
> Ian Jackson writes ("Re: [Xen-devel] xl network-attach SEGV in 4.2 and 4.1"):
>> 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
> 
> IWBNI someone would review this and ack it; I'd like it to go into
> 4.1, 4.2 and unstable.
> 
> Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages]
  2013-04-19  7:12       ` Jan Beulich
@ 2013-04-22  9:49         ` Ian Jackson
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2013-04-22  9:49 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Ian Campbell, Marek Marczykowski

Jan Beulich writes ("Re: [Xen-devel] xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages]"):
> This fixup on 4.1 went wrong - you dropped an & that's necessary.
> I'll take the liberty to fix this right away.

Oops, thanks.

Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list
  2013-04-18 16:01   ` Ian Jackson
  2013-04-18 16:46     ` xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages] Ian Jackson
  2013-04-18 18:58     ` xl network-attach SEGV in 4.2 and 4.1 Roger Pau Monné
@ 2013-11-12 17:23     ` Ian Jackson
  2013-11-12 17:31       ` Ian Campbell
  2 siblings, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2013-11-12 17:23 UTC (permalink / raw)
  To: Ian Campbell, Marek Marczykowski, xen-devel; +Cc: Roger Pau Monne

Ian Jackson writes ("Re: [Xen-devel] xl network-attach SEGV in 4.2 and 4.1"):
> 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.

Ping.

Ian.

>From ea275c0e29299a2399aa7814116d68bfdaeb21c1 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 18 Apr 2013 16:27:46 +0100
Subject: [PATCH] libxl: Avoid realloc(,0) when libxl__xs_directory returns
 empty list
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 tools/libxl/libxl.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0f0f56c..887e0ca 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1864,7 +1864,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;
@@ -2371,7 +2371,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)
@@ -3060,7 +3060,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)
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list
  2013-11-12 17:23     ` [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list Ian Jackson
@ 2013-11-12 17:31       ` Ian Campbell
  2013-11-12 17:57         ` Ian Jackson
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-11-12 17:31 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Roger Pau Monne, Marek Marczykowski, xen-devel

On Tue, 2013-11-12 at 17:23 +0000, Ian Jackson wrote:
> Ian Jackson writes ("Re: [Xen-devel] xl network-attach SEGV in 4.2 and 4.1"):
> > 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.
> 
> Ping.

I could have sworn this went in...

> 
> Ian.
> 
> From ea275c0e29299a2399aa7814116d68bfdaeb21c1 Mon Sep 17 00:00:00 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Thu, 18 Apr 2013 16:27:46 +0100
> Subject: [PATCH] libxl: Avoid realloc(,0) when libxl__xs_directory returns
>  empty list
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> 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.

I thought it was strictly speaking fine, but I can see why it would be
best to avoid.

> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/libxl/libxl.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 0f0f56c..887e0ca 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1864,7 +1864,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;
> @@ -2371,7 +2371,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)
> @@ -3060,7 +3060,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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list
  2013-11-12 17:31       ` Ian Campbell
@ 2013-11-12 17:57         ` Ian Jackson
  2013-11-25 13:55           ` Ian Jackson
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2013-11-12 17:57 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Roger Pau Monne, Marek Marczykowski, xen-devel

Ian Campbell writes ("Re: [PATCH RESEND] libxl: Avoid realloc(,0) when libxl__xs_directory returns empty list"):
> On Tue, 2013-11-12 at 17:23 +0000, Ian Jackson wrote:
> > Ping.
> 
> I could have sworn this went in...

Yes.  I have found a bunch of these lying around in branches in my
main tree.

> > In three places in libxl this may result in a zero size argument being
> > passed to malloc() or realloc(), which is not adviseable.
> 
> I thought it was strictly speaking fine, but I can see why it would be
> best to avoid.

Calling malloc(0) merely produces a fiddly-to-deal-with corner case,
where it may succeed by returning 0.

realloc(something,0) is actually wrong.  If it returns 0 you don't
know whether that means "I tried to allocate you a new 0-length block,
but failed, so your original block is still there" or "I have
successfully freed your old block; have this null pointer as an
answer".

> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

And put on the backport list.

Thanks,
Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list
  2013-11-12 17:57         ` Ian Jackson
@ 2013-11-25 13:55           ` Ian Jackson
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2013-11-25 13:55 UTC (permalink / raw)
  To: Ian Campbell, Marek Marczykowski, xen-devel, Roger Pau Monne

Ian Jackson writes ("Re: [PATCH RESEND] libxl: Avoid realloc(,0) when libxl__xs_directory returns empty list"):
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> And put on the backport list.

Now applied to 4.3.  Was not applicable to 4.2, 4.1.

Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-11-25 13:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-17  2:49 xl network-attach SEGV in 4.2 and 4.1 Marek Marczykowski
2013-04-17  8:43 ` Ian Campbell
2013-04-18 16:01   ` Ian Jackson
2013-04-18 16:46     ` xl network-attach SEGV in 4.2 and 4.1 [and 1 more messages] Ian Jackson
2013-04-19  7:12       ` Jan Beulich
2013-04-22  9:49         ` Ian Jackson
2013-04-18 18:58     ` xl network-attach SEGV in 4.2 and 4.1 Roger Pau Monné
2013-11-12 17:23     ` [PATCH RESEND] libxl: Avoid realloc(, 0) when libxl__xs_directory returns empty list Ian Jackson
2013-11-12 17:31       ` Ian Campbell
2013-11-12 17:57         ` Ian Jackson
2013-11-25 13:55           ` Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.