2010/12/3 Eric Blake <eblake(a)redhat.com&gt;: Is outbuf guaranteed to be non-NULL when virCommandRun succeeds? Otherwise we have a potential NULL dereference here. Is outbuf guaranteed to be unallocated when virCommandRun fails? Otherwise a VIR_FREE(outbuf) is missing here. Same question here about outbuf being guaranteed to be non-NULL when virCommandRun succeeds as in openvzLoadDomains. If not then sscanf is called with NULL and that'll probably segfault. Matthias

popen must be matched with pclose (not fclose), or it will leak resources. Furthermore, it is a lousy interface when it comes to signal handling. We're much better off using our decent command wrapper. Note that virCommand guarantees that VIR_FREE(outbuf) is both required and safe to call, whether virCommandRun succeeded or failed. * src/openvz/openvz_conf.c (openvzLoadDomains, openvzGetVEID): Replace popen with virCommand usage. --- v2: avoid memory leak on failure in openvzGetVEID This patch is awaiting review on these prerequisites: https://www.redhat.com/archives/libvir-list/2010-December/msg00318.html https://www.redhat.com/archives/libvir-list/2010-December/msg00321.html src/openvz/openvz_conf.c | 55 ++++++++++++++++++++++++--------------------- 1 files changed, 29 insertions(+), 26 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 863af93..800c1f4 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -51,6 +51,7 @@ #include "util.h" #include "nodeinfo.h" #include "files.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_OPENVZ @@ -433,26 +434,26 @@ openvzFreeDriver(struct openvz_driver *driver) int openvzLoadDomains(struct openvz_driver *driver) { - FILE *fp; int veid, ret; char status[16]; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr dom = NULL; char temp[50]; + char *outbuf = NULL; + char *line; + virCommandPtr cmd = NULL; if (openvzAssignUUIDs() < 0) return -1; - if ((fp = popen(VZLIST " -a -ovpsid,status -H 2>/dev/null", "r")) == NULL) { - openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); - return -1; - } - - while (!feof(fp)) { - if (fscanf(fp, "%d %s\n", &veid, status) != 2) { - if (feof(fp)) - break; + cmd = virCommandNewArgList(VZLIST, "-a", "-ovpsid,status", "-H", NULL); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + line = *outbuf ? outbuf : NULL; + while (line) { + if (sscanf(line, "%d %s\n", &veid, status) != 2) { openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to parse vzlist output")); goto cleanup; @@ -526,9 +527,14 @@ int openvzLoadDomains(struct openvz_driver *driver) { virDomainObjUnlock(dom); dom = NULL; + + line = strchr(line, '\n'); + if (line) + line++; } - VIR_FORCE_FCLOSE(fp); + virCommandFree(cmd); + VIR_FREE(outbuf); return 0; @@ -536,7 +542,8 @@ int openvzLoadDomains(struct openvz_driver *driver) { virReportOOMError(); cleanup: - VIR_FORCE_FCLOSE(fp); + virCommandFree(cmd); + VIR_FREE(outbuf); if (dom) virDomainObjUnref(dom); return -1; @@ -978,27 +985,23 @@ static int openvzAssignUUIDs(void) */ int openvzGetVEID(const char *name) { - char *cmd; + virCommandPtr cmd; + char *outbuf; int veid; - FILE *fp; bool ok; - if (virAsprintf(&cmd, "%s %s -ovpsid -H", VZLIST, name) < 0) { - openvzError(VIR_ERR_INTERNAL_ERROR, "%s", - _("virAsprintf failed")); + cmd = virCommandNewArgList(VZLIST, name, "-ovpsid", "-H", NULL); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) { + virCommandFree(cmd); + VIR_FREE(outbuf); return -1; } - fp = popen(cmd, "r"); - VIR_FREE(cmd); - - if (fp == NULL) { - openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed")); - return -1; - } + virCommandFree(cmd); + ok = sscanf(outbuf, "%d\n", &veid) == 1; + VIR_FREE(outbuf); - ok = fscanf(fp, "%d\n", &veid ) == 1; - VIR_FORCE_FCLOSE(fp); if (ok && veid >= 0) return veid; -- 1.7.3.2

* src/openvz/openvz_conf.c (openvzLoadDomains): Replace unsafe sscanf with safe direct parsing. (openvzGetVEID): Avoid lost integer overflow detection. (openvzAssignUUIDs): Likewise, and detect readdir failure. --- v2: new patch; plugs a potential security hole, since *scanf("%s",fixed_width_buffer) is exploitable, but the exploit could only happen if /usr/sbin/vzlist is compromised. src/openvz/openvz_conf.c | 39 +++++++++++++++++++++++++-------------- 1 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 800c1f4..2d1c41a 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -435,7 +435,7 @@ openvzFreeDriver(struct openvz_driver *driver) int openvzLoadDomains(struct openvz_driver *driver) { int veid, ret; - char status[16]; + char *status; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr dom = NULL; char temp[50]; @@ -451,13 +451,17 @@ int openvzLoadDomains(struct openvz_driver *driver) { if (virCommandRun(cmd, NULL) < 0) goto cleanup; - line = *outbuf ? outbuf : NULL; - while (line) { - if (sscanf(line, "%d %s\n", &veid, status) != 2) { + line = outbuf; + while (line[0] != '\0') { + if (virStrToLong_i(line, &status, 10, &veid) < 0 || + *status++ != ' ' || + (line = strchr(status, '\n')) == NULL) { openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to parse vzlist output")); goto cleanup; } + *line++ = '\0'; if (VIR_ALLOC(dom) < 0) goto no_memory; @@ -527,10 +531,6 @@ int openvzLoadDomains(struct openvz_driver *driver) { virDomainObjUnlock(dom); dom = NULL; - - line = strchr(line, '\n'); - if (line) - line++; } virCommandFree(cmd); @@ -953,8 +953,9 @@ static int openvzAssignUUIDs(void) DIR *dp; struct dirent *dent; char *conf_dir; - int vpsid, res; - char ext[8]; + int vpsid; + char *ext; + int ret = 0; conf_dir = openvzLocateConfDir(); if (conf_dir == NULL) @@ -966,16 +967,25 @@ static int openvzAssignUUIDs(void) return 0; } + errno = 0; while ((dent = readdir(dp))) { - res = sscanf(dent->d_name, "%d.%5s", &vpsid, ext); - if (!(res == 2 && STREQ(ext, "conf"))) + if (virStrToLong_i(dent->d_name, &ext, 10, &vpsid) < 0 || + *ext++ != '.' || + STRNEQ(ext, "conf")) continue; if (vpsid > 0) /* '0.conf' belongs to the host, ignore it */ openvzSetUUID(vpsid); + errno = 0; + } + if (errno) { + openvzError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to scan configuration directory")); + ret = -1; } + closedir(dp); VIR_FREE(conf_dir); - return 0; + return ret; } @@ -987,6 +997,7 @@ static int openvzAssignUUIDs(void) int openvzGetVEID(const char *name) { virCommandPtr cmd; char *outbuf; + char *temp; int veid; bool ok; @@ -999,7 +1010,7 @@ int openvzGetVEID(const char *name) { } virCommandFree(cmd); - ok = sscanf(outbuf, "%d\n", &veid) == 1; + ok = virStrToLong_i(outbuf, &temp, 10, &veid) == 0 && *temp == '\n'; VIR_FREE(outbuf); if (ok && veid >= 0) -- 1.7.3.2