2010/12/3 Eric Blake <eblake@xxxxxxxxxx>:
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list