Re: [Xen-devel] [PATCH v2 1/4] minios: correct char array allocation for xenbus paths

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-09-06 at 12:52 -0700, Matt Wilson wrote:
> From: Matt Wilson <msw@xxxxxxxxxx>
> 
> The char arrays used to hold xenbus paths have historically been
> allocated by manually counting the length longest string constants
> included in constructing the path. This has led to improperly sized
> buffers, both too large (with little consequence) and too small (which
> obviously causes problems). This patch corrects the instances where
> the length was incorrectly calculated by using strlen() on the longest
> string constant used in building a xenbus path.
> 
> A follow-on clean-up patch will change all instances to use strlen().
> 
> Signed-off-by: Ben Cressey <bcressey@xxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
> [msw: split this patch from a larger patch from Ben, reworked to use
>  strlen()]
> Signed-off-by: Matt Wilson <msw@xxxxxxxxxx>

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

> ---
>  extras/mini-os/blkfront.c       |    2 +-
>  extras/mini-os/console/xenbus.c |    2 +-
>  extras/mini-os/fbfront.c        |   10 +++++-----
>  extras/mini-os/netfront.c       |    2 +-
>  extras/mini-os/pcifront.c       |    7 +++++--
>  5 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/extras/mini-os/blkfront.c b/extras/mini-os/blkfront.c
> index f4283a9..70976f5 100644
> --- a/extras/mini-os/blkfront.c
> +++ b/extras/mini-os/blkfront.c
> @@ -254,7 +254,7 @@ void shutdown_blkfront(struct blkfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
>  
>      blkfront_sync(dev);
>  
> diff --git a/extras/mini-os/console/xenbus.c b/extras/mini-os/console/xenbus.c
> index e65baf7..1ecfcc5 100644
> --- a/extras/mini-os/console/xenbus.c
> +++ b/extras/mini-os/console/xenbus.c
> @@ -158,7 +158,7 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 19 + 1];
> +        char path[strlen(dev->backend) + strlen("/state") + 1];
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>          
>       xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
> diff --git a/extras/mini-os/fbfront.c b/extras/mini-os/fbfront.c
> index 54a5e67..6eddb3c 100644
> --- a/extras/mini-os/fbfront.c
> +++ b/extras/mini-os/fbfront.c
> @@ -158,8 +158,8 @@ done:
>  
>      {
>          XenbusState state;
> -        char path[strlen(dev->backend) + 1 + 6 + 1];
> -        char frontpath[strlen(nodename) + 1 + 6 + 1];
> +        char path[strlen(dev->backend) + strlen("/state") + 1];
> +        char frontpath[strlen(nodename) + strlen("/state") + 1];
>  
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
> @@ -240,7 +240,7 @@ void shutdown_kbdfront(struct kbdfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/request-abs-pointer") + 
> 1];
>  
>      printk("close kbd: backend at %s\n",dev->backend);
>  
> @@ -521,7 +521,7 @@ done:
>      {
>          XenbusState state;
>          char path[strlen(dev->backend) + 1 + 14 + 1];
> -        char frontpath[strlen(nodename) + 1 + 6 + 1];
> +        char frontpath[strlen(nodename) + strlen("/state") + 1];
>  
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
> @@ -632,7 +632,7 @@ void shutdown_fbfront(struct fbfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/feature-update") + 1];
>  
>      printk("close fb: backend at %s\n",dev->backend);
>  
> diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
> index 6fa68a2..ddf56ea 100644
> --- a/extras/mini-os/netfront.c
> +++ b/extras/mini-os/netfront.c
> @@ -508,7 +508,7 @@ void shutdown_netfront(struct netfront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/request-rx-copy") + 1];
>  
>      printk("close network: backend at %s\n",dev->backend);
>  
> diff --git a/extras/mini-os/pcifront.c b/extras/mini-os/pcifront.c
> index bbe21e0..f9ae768 100644
> --- a/extras/mini-os/pcifront.c
> +++ b/extras/mini-os/pcifront.c
> @@ -323,7 +323,7 @@ void shutdown_pcifront(struct pcifront_dev *dev)
>      XenbusState state;
>  
>      char path[strlen(dev->backend) + 1 + 5 + 1];
> -    char nodename[strlen(dev->nodename) + 1 + 5 + 1];
> +    char nodename[strlen(dev->nodename) + strlen("/event-channel") + 1];
>  
>      printk("close pci: backend at %s\n",dev->backend);
>  
> @@ -379,7 +379,10 @@ int pcifront_physical_to_virtual (struct pcifront_dev 
> *dev,
>                                    unsigned int *slot,
>                                    unsigned long *fun)
>  {
> -    char path[strlen(dev->backend) + 1 + 5 + 10 + 1];
> +    /* FIXME: the buffer sizing is a little lazy here. 10 extra bytes
> +       should be enough to hold the paths we need to construct, even
> +       if the number of devices is large */
> +    char path[strlen(dev->backend) + strlen("/num_devs") + 10 + 1];
>      int i, n;
>      char *s, *msg = NULL;
>      unsigned int dom1, bus1, slot1, fun1;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel