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@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@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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Chris Lalancette wrote:

On 12/14/2009 02:48 PM, Jim Meyering wrote:

...

From: Jim Meyering <meyering@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) {

FYI; I found out today that virFileResolveLink() doesn't work on filesystems backed by sysfs (which unfortunately this code path is). The problem is that sysfs doesn't follow the POSIX-specified behavior of placing the size of the name of the real path in st.st_size; instead, on sysfs, st.st_size for symlinks is *always* 0 (at least on my F-12 box here). So this code path is probably broken now. DV said he will take a look at putting a patch together to make virFileResolveLink() handle the 0 case.

Good point. Don't bother rolling your own. I'll prepare a patch to use gnulib's areadlink module.