Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  hw/qdev.c |   14 +++++++++++++-
>  vl.c      |    7 ++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index af54467..f1e83a4 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, 
> char *p, int size)
>      if (dev && dev->parent_bus) {
>          char *d;
>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
> +        if (l >= size) {
> +            return l;
> +        }
> +
>          d = bus_get_fw_dev_path(dev->parent_bus, dev);
>          if (d) {
>              l += snprintf(p + l, size - l, "%s", d);
> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, 
> char *p, int size)
>          } else {
>              l += snprintf(p + l, size - l, "%s", 
> object_get_typename(OBJECT(dev)));
>          }
> +
> +        if (l >= size) {
> +            return l;
> +        }
>      }
>      l += snprintf(p + l , size - l, "/");
>  

If the return value is less than the size argument, it's the length of
the string written into p[].  Else, it means p[] has insufficient
space.

> @@ -520,8 +528,12 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
>      char path[128];
>      int l;
>  
> -    l = qdev_get_fw_dev_path_helper(dev, path, 128);
> +    l = qdev_get_fw_dev_path_helper(dev, path, sizeof(path));
>  
> +    assert(l > 0);
> +    if (l >= sizeof(path)) {
> +        return NULL;
> +    }

Failure mode could be avoided with the common technique: make
qdev_get_fw_dev_path_helper() return the true length.  If it fit into
path[], return strdup(path).  Else, allocate a suitable buffer and try
again.

What do you think?

>      path[l-1] = '\0';
>  
>      return strdup(path);
> diff --git a/vl.c b/vl.c
> index 8904db1..78dcc93 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -913,7 +913,12 @@ char *get_boot_devices_list(uint32_t *size)
>  
>          if (i->dev) {
>              devpath = qdev_get_fw_dev_path(i->dev);
> -            assert(devpath);
> +            if (devpath == NULL) {
> +                fprintf(stderr,
> +                        "OpenFirmware Device Path too long (boot index 
> %d)\n",
> +                        i->bootindex);
> +                exit(1);
> +            }
>          }
>  
>          if (i->suffix && devpath) {