[libvirt] [PATCH] openvz: convert popen to virCommand
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. * src/openvz/openvz_conf.c (openvzLoadDomains, openvzGetVEID): Replace popen with virCommand usage. --- src/openvz/openvz_conf.c | 54 +++++++++++++++++++++++---------------------- 1 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 863af93..9d2689a 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,22 @@ 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); 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
2010/12/3 Eric Blake <eblake@redhat.com>:
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.
* src/openvz/openvz_conf.c (openvzLoadDomains, openvzGetVEID): Replace popen with virCommand usage. --- src/openvz/openvz_conf.c | 54 +++++++++++++++++++++++---------------------- 1 files changed, 28 insertions(+), 26 deletions(-)
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;
Is outbuf guaranteed to be non-NULL when virCommandRun succeeds? Otherwise we have a potential NULL dereference here.
+ while (line) { + if (sscanf(line, "%d %s\n", &veid, status) != 2) { openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to parse vzlist output")); goto cleanup;
@@ -978,27 +985,22 @@ 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);
Is outbuf guaranteed to be unallocated when virCommandRun fails? Otherwise a VIR_FREE(outbuf) is missing here.
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;
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.
+ VIR_FREE(outbuf);
- ok = fscanf(fp, "%d\n", &veid ) == 1; - VIR_FORCE_FCLOSE(fp); if (ok && veid >= 0) return veid;
Matthias
On 12/04/2010 04:21 AM, Matthias Bolte wrote:
2010/12/3 Eric Blake <eblake@redhat.com>:
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.
+ line = *outbuf ? outbuf : NULL;
Is outbuf guaranteed to be non-NULL when virCommandRun succeeds? Otherwise we have a potential NULL dereference here.
Yep, I had already noticed that, which is why I posted two versions of a fix to virCommand to cover alternative approaches to this problem (only the v2 fix would guarantee non-NULL here).
+ if (virCommandRun(cmd, NULL) < 0) { + virCommandFree(cmd);
Is outbuf guaranteed to be unallocated when virCommandRun fails? Otherwise a VIR_FREE(outbuf) is missing here.
Not yet, but I think it should be (in other words, I need a v3 of my virCommand fix that guarantees: on success, if virCommandSet{Output,Error}Buffer was called, then that buffer will be allocated to a non-NULL string, and caller is responsible for freeing it on failure, then those buffers are guaranteed to be NULL (if the caller uses VIR_FREE, it won't hurt, but they do not have to explicitly worry about partial data being collected on failure)
+ virCommandFree(cmd); + ok = sscanf(outbuf, "%d\n", &veid) == 1;
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.
That sscanf is overkill anyways; I can probably convert both *scanf touched by this patch into lower-level functions, and in the process, avoid problems such as ignoring overflow that are inherent in scanf. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
2010/12/7 Eric Blake <eblake@redhat.com>:
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(-)
ACK. Matthias
* 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
2010/12/7 Eric Blake <eblake@redhat.com>:
* 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(-)
ACK. Matthias
On 12/07/2010 02:49 PM, Matthias Bolte wrote:
2010/12/7 Eric Blake <eblake@redhat.com>:
* 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(-)
ACK.
Thanks; I've pushed the series. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Matthias Bolte