4:59 a.m.
On Fri, Aug 07, 2009 at 11:06:29AM +0200, Chris Lalancette wrote:
diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c
index e008a7f..42084c7 100644
--- a/proxy/libvirt_proxy.c
+++ b/proxy/libvirt_proxy.c
@@ -167,10 +167,13 @@ proxyListenUnixSocket(const char *path) {
* Abstract socket do not hit the filesystem, way more secure and
* guaranteed to be atomic
*/
- memset(&addr, 0, sizeof(addr));
addr.sun_family = AF_UNIX;
addr.sun_path[0] = '\0';
- strncpy(&addr.sun_path[1], path, (sizeof(addr) - 4) - 2);
+ if (virStrcpy(&addr.sun_path[1], path, sizeof(addr.sun_path) - 1) == NULL) {
+ fprintf(stderr, "Path %s too long to fit into destination\n", path);
+ close(fd);
+ return -1;
+ }
Removing the memset is not safe - the abstract namesapce defines
the name to be the *entire* addr.sun_path array. So it must be
null filled, otherwise the name will be the string copied + whatever
trailing garbage exists
diff --git a/qemud/qemud.c b/qemud/qemud.c
index b2bd59d..17fc172 100644
--- a/qemud/qemud.c
+++ b/qemud/qemud.c
@@ -509,13 +509,14 @@ static int qemudListenUnix(struct qemud_server *server,
virSetNonBlock(sock->fd) < 0)
goto cleanup;
- memset(&addr, 0, sizeof(addr));
addr.sun_family = AF_UNIX;
- strncpy(addr.sun_path, path, sizeof(addr.sun_path)-1);
+ if (virStrcpy(addr.sun_path, path, sizeof(addr.sun_path)) == NULL) {
+ VIR_ERROR(_("Path %s too long for unix socket"), path);
+ goto cleanup;
+ }
if (addr.sun_path[0] == '@')
addr.sun_path[0] = '\0';
Likewise removing memset() here is not safe
diff --git a/src/lxc_controller.c b/src/lxc_controller.c
index 8d11238..6bc9976 100644
--- a/src/lxc_controller.c
+++ b/src/lxc_controller.c
@@ -176,9 +176,12 @@ static int lxcMonitorServer(const char *sockpath)
}
unlink(sockpath);
- memset(&addr, 0, sizeof(addr));
addr.sun_family = AF_UNIX;
- strncpy(addr.sun_path, sockpath, sizeof(addr.sun_path));
+ if (virStrcpy(addr.sun_path, sockpath, sizeof(addr.sun_path)) == NULL) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Socket path %s too long for destination"), sockpath);
+ goto error;
+ }
Another memset().
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 37fdec2..bcacd41 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -909,7 +909,11 @@ qemudOpenMonitorUnix(virConnectPtr conn,
memset(&addr, 0, sizeof(addr));
addr.sun_family = AF_UNIX;
- strncpy(addr.sun_path, monitor, sizeof(addr.sun_path));
+ if (virStrcpy(addr.sun_path, monitor, sizeof(addr.sun_path)) == NULL) {
+ qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("Monitor path %s too big for destination"),
monitor);
+ goto error;
+ }
do {
ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr));
@@ -5802,8 +5806,11 @@ static int qemuGetSchedulerParameters(virDomainPtr dom,
goto cleanup;
}
params[0].value.ul = val;
- strncpy(params[0].field, "cpu_shares", sizeof(params[0].field));
- params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG;
+ if (virStrcpy(params[0].field, "cpu_shares", sizeof(params[0].field)) ==
NULL) {
+ qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Field cpu_shares too long for
destination"));
+ goto cleanup;
+ }
Why remove the assginemnt to 'type' here ?
@@ -1573,6 +1572,28 @@ virAsprintf(char **strp, const char *fmt,
...)
return ret;
}
+/**
+ * virStrcpy
+ *
+ * A safe version of strcpy. The last parameter is the number of bytes
+ * available in the destination string, *not* the number of bytes you want
+ * to copy. If the destination is not large enough to hold all of the
+ * src string, NULL is returned and no data is copied. If the destination
+ * is large enough to hold all of the src, then the string is copied and
+ * a pointer to the destination string is returned.
+ */
+char *
+virStrcpy(char *dest, const char *src, size_t destbytes)
+{
+ int len;
+
+ len = strlen(src) + 1;
+ if (len > destbytes)
+ return NULL;
+
+ return memcpy(dest, src, len);
+}
Why not just use strncpy() and ensure the trailing null. Seems
like it would be more efficient than requiring 2 passes over
the string.
index b3e628a..ffbf2b3 100644
--- a/src/util.h
+++ b/src/util.h
@@ -164,6 +164,7 @@ void virSkipSpaces(const char **str);
int virParseNumber(const char **str);
int virAsprintf(char **strp, const char *fmt, ...)
ATTRIBUTE_FMT_PRINTF(2, 3);
+char *virStrcpy(char *dest, const char *src, size_t destbytes);
THis is probably a candidate for ATTRIBUTE_RETURN_CHECK to
guarentee that all callers are dealing with the failure case
at compile time
@@ -1026,42 +1043,46 @@ xenXMDomainConfigParse(virConnectPtr conn,
virConfPtr conf) {
data++;
if (STRPREFIX(key, "mac=")) {
- int len = nextkey ? (nextkey - data) : 17;
- if (len > 17)
- len = 17;
- strncpy(mac, data, len);
- mac[len] = '\0';
+ if (virStrcpy(mac, data, sizeof(mac)) == NULL) {
+ xenXMError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("MAC address %s too big for
destination"),
+ data);
+ goto cleanup;
+ }
This doesn't seem correct to me, since 'data' is not NULL terminated
at the place we want the copy to stop. The string is of the form
foo=bar,mac=XYZ,foo=bar
and the original code would only copy XYZ, while this new code
will copy the maximum size of the destination buffer, or the
remaining of the source string, including the next field.
I'm surprised if the xmconfigtest test case passes with this,
unless i'm mis-reading something
} else if (STRPREFIX(key, "bridge=")) {
- int len = nextkey ? (nextkey - data) : sizeof(bridge)-1;
+ if (virStrcpy(bridge, data, sizeof(bridge)) == NULL) {
+ xenXMError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("Bridge %s too big for destination"),
+ data);
+ goto cleanup;
+ }
This appears wrong to me for similar reasons as above.. and
the remainder of this files changes
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|