[libvirt] [PATCH] node_device_driver.c: don't write beyond EOB for 4K-byte symlink

[Daniel Veillard]

On Mon, Dec 14, 2009 at 02:48:51PM +0100, Jim Meyering wrote:
> Jim Meyering wrote:
> > Without this patch, a symlink pointing to a 4096-byte name
> > could make this code write NUL into the byte beyond end of buffer:
> >
> >     if ((n = readlink(driver_link, devpath, sizeof devpath - 1)) < 0) {
> >         virReportSystemError(conn, errno,
> >                              _("cannot resolve driver link %s"), driver_link);
> >         goto cleanup;
> >     }
> >     devpath[n] = '\0';
> >
> >
> >>From a075e207bc8fb279c43c9f4f43a960ffbd9a8a70 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering at redhat.com>
> > Date: Mon, 14 Dec 2009 12:05:38 +0100
> > Subject: [PATCH] node_device_driver.c: don't write beyond EOB for 4K-byte symlink
> >
> > * src/node_device/node_device_driver.c (update_driver_name): Leave
> > one byte for the trailing NUL we'll append.
> > ---
> >  src/node_device/node_device_driver.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> > index f083f16..eda5d5e 100644
> > --- a/src/node_device/node_device_driver.c
> > +++ b/src/node_device/node_device_driver.c
> > @@ -97,7 +97,7 @@ static int update_driver_name(virConnectPtr conn,
> >          goto cleanup;
> >      }
> >
> > -    if ((n = readlink(driver_link, devpath, sizeof devpath)) < 0) {
> > +    if ((n = readlink(driver_link, devpath, sizeof devpath - 1)) < 0) {
> >          virReportSystemError(conn, errno,
> >                               _("cannot resolve driver link %s"), driver_link);
> >          goto cleanup;
> 
> The above is correct, but Daniel Veillard suggested a better (albeit
> slightly larger) change: use virFileResolveLink instead of readlink:
> 
> >From 4ae050481d481629fc98e8e7f5322ce6d724d3f7 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Mon, 14 Dec 2009 12:05:38 +0100
> Subject: [PATCH] node_device_driver.c: don't write beyond EOB for 4K-byte symlink
> 
> * src/node_device/node_device_driver.c (update_driver_name): The
> previous code would write one byte beyond the end of the 4KiB
> stack buffer when presented with a symlink value of exactly that
> length (very unlikely).  Remove the automatic buffer and use
> virFileResolveLink in place of readlink.  Suggested by Daniel Veillard.
> ---
>  src/node_device/node_device_driver.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index f083f16..ecbac0f 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -78,10 +78,9 @@ static int update_driver_name(virConnectPtr conn,
>                                virNodeDeviceObjPtr dev)
>  {
>      char *driver_link = NULL;
> -    char devpath[PATH_MAX];
> +    char *devpath;
>      char *p;
>      int ret = -1;
> -    int n;
> 
>      VIR_FREE(dev->def->driver);
> 
> @@ -97,12 +96,11 @@ static int update_driver_name(virConnectPtr conn,
>          goto cleanup;
>      }
> 
> -    if ((n = readlink(driver_link, devpath, sizeof devpath)) < 0) {
> +    if (virFileResolveLink(driver_link, &devpath) < 0) {
>          virReportSystemError(conn, errno,
>                               _("cannot resolve driver link %s"), driver_link);
>          goto cleanup;
>      }
> -    devpath[n] = '\0';
> 
>      p = strrchr(devpath, '/');
>      if (p) {
> @@ -116,6 +114,7 @@ static int update_driver_name(virConnectPtr conn,
> 
>  cleanup:
>      VIR_FREE(driver_link);
> +    free(devpath);
>      return ret;
>  }
>  #else

  ACK, thanks Jim !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/