[libvirt] [PATCH] openvzGetVEID: don't leak (memory + file descriptor)

Coverity spotted the leaks. Here's a proposed fix. I didn't bother with a separate diagnostic for the unlikely event that we read a negative number from the pipe. Seems far-fetched enough not to bother, but it's easy to add, if anyone cares.
From f3439c7eae46681eacf5b469a6f0e22cb8fec1b4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 25 Feb 2010 19:24:50 +0100 Subject: [PATCH] openvzGetVEID: don't leak (memory + file descriptor)
* src/openvz/openvz_conf.c (openvzGetVEID): Always call fclose. Diagnose parse failure also when vzlist output is empty. If somehow we read a -1, diagnose that (albeit as a parse failure). --- src/openvz/openvz_conf.c | 19 +++++++------------ 1 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index ce0c4d3..3713a45 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -964,6 +964,7 @@ int openvzGetVEID(const char *name) { char *cmd; int veid; FILE *fp; + bool ok; if (virAsprintf(&cmd, "%s %s -ovpsid -H", VZLIST, name) < 0) { openvzError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", @@ -979,18 +980,12 @@ int openvzGetVEID(const char *name) { return -1; } - if (fscanf(fp, "%d\n", &veid ) != 1) { - if (feof(fp)) - return -1; - - openvzError(NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to parse vzlist output")); - goto cleanup; - } - - return veid; - - cleanup: + ok = fscanf(fp, "%d\n", &veid ) == 1; fclose(fp); + if (ok && veid >= 0) + return veid; + + openvzError(NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to parse vzlist output")); return -1; } -- 1.7.0.401.g84adb

According to Jim Meyering on 2/25/2010 11:30 AM: ACK on plugging the leak. However,...
@@ -979,18 +980,12 @@ int openvzGetVEID(const char *name) { return -1; }
- if (fscanf(fp, "%d\n", &veid ) != 1) { + ok = fscanf(fp, "%d\n", &veid ) == 1;
You're still keeping with fscanf. Isn't that dangerous, since fscanf is undefined in the presence of integer overflow (that is, if fp sends more decimal digits than fit in veid)? This seems like one of the reasons that coreutils completely prohibits *scanf (another being buffer overflow exploits with %s, but that's not relevant to this chunk of code). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Eric Blake wrote:
According to Jim Meyering on 2/25/2010 11:30 AM:
ACK on plugging the leak. However,...
Thanks for the review.
@@ -979,18 +980,12 @@ int openvzGetVEID(const char *name) { return -1; }
- if (fscanf(fp, "%d\n", &veid ) != 1) { + ok = fscanf(fp, "%d\n", &veid ) == 1;
You're still keeping with fscanf.
Yes. There are plenty of bigger fish to fry, for now ;-) As it is, this is more of a rearrangement than I generally prefer "just to fix a leak".
Isn't that dangerous, since fscanf is undefined in the presence of integer overflow (that is, if fp sends more
True, if it reads 4294967296 (2^32), fscanf is almost guaranteed -- in practice, not by any standard, of course -- to interpret it as 0.
decimal digits than fit in veid)? This seems like one of the reasons that coreutils completely prohibits *scanf (another being buffer overflow exploits with %s, but that's not relevant to this chunk of code).
If we could magically remove all uses of *scanf, atoi, etc., that'd be great ;-) But making so many changes is a little risky now, considering how little test coverage we have.

According to Jim Meyering on 2/25/2010 12:01 PM:
You're still keeping with fscanf.
Yes. There are plenty of bigger fish to fry, for now ;-) As it is, this is more of a rearrangement than I generally prefer "just to fix a leak".
Indeed, I have no problem with checking in your patch as-is, and saving the *scanf cleanups for a separate patch.
If we could magically remove all uses of *scanf, atoi, etc., that'd be great ;-) But making so many changes is a little risky now, considering how little test coverage we have.
Sounds like I've just volunteered myself for a beginner's project then. It may be a while (and probably should wait until after the current release), but it seems like a patch series that addresses dangerous constructs one place at a time would be worthwhile, both in improving the code base and in getting me familiar with more than just the autotools input files. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Jim Meyering