[libvirt] [PATCH 5/7] lxc: avoid large stacks with veth creation
Eric Blake
eblake at redhat.com
Wed Sep 1 19:59:57 UTC 2010
* src/lxc/veth.h (vethCreate): Change prototype.
* src/lxc/veth.c (vethCreate): Always malloc veth2, and allocate
veth1 if needed.
(getFreeVethName): Adjust signature, and use virAsprintf.
* src/lxc/lxc_driver.c (lxcSetupInterfaces): Adjust caller.
---
This issue crossed file boundaries. It was a bit tricky, since
vethCreate was used in two patterns - one where the parent name was
already known, and another where the parent name is picked as the
first available option. But the end result avoids strdup'ing a
fixed-width buffer, and I think I correctly avoided any leaks (in
lxcSetupInterfaces, once a string is transferred to *veths, then
returning failure will cause that string to be freed in the caller).
It also gets rid of a PATH_MAX stack over-allocation.
src/lxc/lxc_driver.c | 32 ++++++++-----------
src/lxc/veth.c | 84 ++++++++++++++++++++++++++++++-------------------
src/lxc/veth.h | 6 ++--
3 files changed, 67 insertions(+), 55 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 6fe20b1..326fee6 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -812,14 +812,16 @@ static int lxcSetupInterfaces(virConnectPtr conn,
return -1;
for (i = 0 ; i < def->nnets ; i++) {
- char parentVeth[PATH_MAX] = "";
- char containerVeth[PATH_MAX] = "";
+ char *parentVeth;
+ char *containerVeth = NULL;
switch (def->nets[i]->type) {
case VIR_DOMAIN_NET_TYPE_NETWORK:
{
- virNetworkPtr network = virNetworkLookupByName(conn,
- def->nets[i]->data.network.name);
+ virNetworkPtr network;
+
+ network = virNetworkLookupByName(conn,
+ def->nets[i]->data.network.name);
if (!network) {
goto error_exit;
}
@@ -852,31 +854,23 @@ static int lxcSetupInterfaces(virConnectPtr conn,
}
DEBUG0("calling vethCreate()");
- if (NULL != def->nets[i]->ifname) {
- strcpy(parentVeth, def->nets[i]->ifname);
- }
- DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
- if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0)
+ parentVeth = def->nets[i]->ifname;
+ if (vethCreate(&parentVeth, &containerVeth) < 0)
goto error_exit;
+ DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
if (NULL == def->nets[i]->ifname) {
- def->nets[i]->ifname = strdup(parentVeth);
+ def->nets[i]->ifname = parentVeth;
}
+
if (VIR_REALLOC_N(*veths, (*nveths)+1) < 0) {
virReportOOMError();
+ VIR_FREE(containerVeth);
goto error_exit;
}
- if (((*veths)[(*nveths)] = strdup(containerVeth)) == NULL) {
- virReportOOMError();
- goto error_exit;
- }
+ (*veths)[(*nveths)] = containerVeth;
(*nveths)++;
- if (NULL == def->nets[i]->ifname) {
- virReportOOMError();
- goto error_exit;
- }
-
{
char macaddr[VIR_MAC_STRING_BUFLEN];
virFormatMacAddr(def->nets[i]->mac, macaddr);
diff --git a/src/lxc/veth.c b/src/lxc/veth.c
index 5f038d6..14cfaa2 100644
--- a/src/lxc/veth.c
+++ b/src/lxc/veth.c
@@ -13,6 +13,7 @@
#include <config.h>
#include <string.h>
+#include <stdbool.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -33,42 +34,46 @@
/* Functions */
/**
* getFreeVethName:
- * @veth: name for veth device (NULL to find first open)
- * @maxLen: max length of veth name
+ * @veth: pointer to store returned name for veth device
* @startDev: device number to start at (x in vethx)
*
* Looks in /sys/class/net/ to find the first available veth device
* name.
*
- * Returns 0 on success or -1 in case of error
+ * Returns non-negative device number on success or -1 in case of error
*/
-static int getFreeVethName(char *veth, int maxLen, int startDev)
+static int getFreeVethName(char **veth, int startDev)
{
- int rc = -1;
int devNum = startDev-1;
- char path[PATH_MAX];
+ char *path = NULL;
do {
+ VIR_FREE(path);
++devNum;
- snprintf(path, PATH_MAX, "/sys/class/net/veth%d/", devNum);
+ if (virAsprintf(&path, "/sys/class/net/veth%d/", devNum) < 0) {
+ virReportOOMError();
+ return -1;
+ }
} while (virFileExists(path));
+ VIR_FREE(path);
- snprintf(veth, maxLen, "veth%d", devNum);
-
- rc = devNum;
-
- return rc;
+ if (virAsprintf(veth, "veth%d", devNum) < 0) {
+ virReportOOMError();
+ return -1;
+ }
+ return devNum;
}
/**
* vethCreate:
- * @veth1: name for one end of veth pair
- * @veth1MaxLen: max length of veth1 name
- * @veth2: name for one end of veth pair
- * @veth2MaxLen: max length of veth1 name
+ * @veth1: pointer to name for parent end of veth pair
+ * @veth2: pointer to return name for container end of veth pair
*
* Creates a veth device pair using the ip command:
* ip link add veth1 type veth peer name veth2
+ * If veth1 points to NULL on entry, it will be a valid interface on
+ * return. veth2 should point to NULL on entry.
+ *
* NOTE: If veth1 and veth2 names are not specified, ip will auto assign
* names. There seems to be two problems here -
* 1) There doesn't seem to be a way to determine the names of the
@@ -78,43 +83,56 @@ static int getFreeVethName(char *veth, int maxLen, int startDev)
* 2) Once one of the veth devices is moved to another namespace, it
* is no longer visible in the parent namespace. This seems to
* confuse the name assignment causing it to fail with File exists.
- * Because of these issues, this function currently forces the caller
- * to fully specify the veth device names.
+ * Because of these issues, this function currently allocates names
+ * prior to using the ip command, and returns any allocated names
+ * to the caller.
*
* Returns 0 on success or -1 in case of error
*/
-int vethCreate(char* veth1, int veth1MaxLen,
- char* veth2, int veth2MaxLen)
+int vethCreate(char** veth1, char** veth2)
{
int rc;
const char *argv[] = {
- "ip", "link", "add", veth1, "type", "veth", "peer", "name", veth2, NULL
+ "ip", "link", "add", NULL, "type", "veth", "peer", "name", NULL, NULL
};
int cmdResult = 0;
int vethDev = 0;
+ bool veth1_alloc = false;
- DEBUG("veth1: %s veth2: %s", veth1, veth2);
+ DEBUG("veth1: %s veth2: %s", NULLSTR(*veth1), NULLSTR(*veth2));
- while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) {
- vethDev = getFreeVethName(veth1, veth1MaxLen, 0);
- ++vethDev;
- DEBUG("Assigned veth1: %s", veth1);
+ if (*veth1 == NULL) {
+ vethDev = getFreeVethName(veth1, vethDev);
+ if (vethDev < 0)
+ return vethDev;
+ DEBUG("Assigned veth1: %s", *veth1);
+ veth1_alloc = true;
}
-
- while ((1 > strlen(veth2)) || STREQ(veth1, veth2)) {
- vethDev = getFreeVethName(veth2, veth2MaxLen, vethDev);
- ++vethDev;
- DEBUG("Assigned veth2: %s", veth2);
+ argv[3] = *veth1;
+
+ while (*veth2 == NULL || STREQ(*veth1, *veth2)) {
+ VIR_FREE(*veth2);
+ vethDev = getFreeVethName(veth2, vethDev + 1);
+ if (vethDev < 0) {
+ if (veth1_alloc)
+ VIR_FREE(*veth1);
+ return vethDev;
+ }
+ DEBUG("Assigned veth2: %s", *veth2);
}
+ argv[8] = *veth2;
- DEBUG("veth1: %s veth2: %s", veth1, veth2);
+ DEBUG("veth1: %s veth2: %s", *veth1, *veth2);
rc = virRun(argv, &cmdResult);
if (rc != 0 ||
(WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
vethError(VIR_ERR_INTERNAL_ERROR,
_("Failed to create veth device pair '%s', '%s': %d"),
- veth1, veth2, WEXITSTATUS(cmdResult));
+ *veth1, *veth2, WEXITSTATUS(cmdResult));
+ if (veth1_alloc)
+ VIR_FREE(*veth1);
+ VIR_FREE(*veth2);
rc = -1;
}
diff --git a/src/lxc/veth.h b/src/lxc/veth.h
index 1ec1ec8..f50a939 100644
--- a/src/lxc/veth.h
+++ b/src/lxc/veth.h
@@ -1,6 +1,7 @@
/*
* veth.h: Interface to tools for managing veth pairs
*
+ * Copyright (C) 2010 Red Hat, Inc.
* Copyright IBM Corp. 2008
*
* See COPYING.LIB for the License of this software
@@ -16,9 +17,8 @@
# include "internal.h"
/* Function declarations */
-int vethCreate(char* veth1, int veth1MaxLen, char* veth2,
- int veth2MaxLen)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+int vethCreate(char** veth1, char** veth2)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int vethDelete(const char* veth)
ATTRIBUTE_NONNULL(1);
int vethInterfaceUpOrDown(const char* veth, int upOrDown)
--
1.7.2.2
More information about the libvir-list
mailing list