[libvirt] [PATCH] cgroup: avoid leaking a file
Clang detected a dead store to rc. It turns out that in fixing this, I also found a FILE* leak. * src/util/cgroup.c (virCgroupKillInternal): Abort rather than resuming loop on fscanf failure, and cleanup file on error. --- As a bonus, this also fixes a decl-after-statement for fp. src/util/cgroup.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index afe8731..62b371d 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1351,7 +1351,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr int killedAny = 0; char *keypath = NULL; bool done = false; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); + FILE *fp = NULL; + VIR_DEBUG("group=%p path=%s signum=%d pids=%p", + group, group->path, signum, pids); rc = virCgroupPathOfController(group, -1, "tasks", &keypath); if (rc != 0) { @@ -1364,7 +1366,6 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr */ while (!done) { done = true; - FILE *fp; if (!(fp = fopen(keypath, "r"))) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath); @@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr if (feof(fp)) break; rc = -errno; - break; + VIR_DEBUG("Failed to read %s: %m\n", keypath); + goto cleanup; } if (virHashLookup(pids, (void*)pid)) continue; @@ -1403,6 +1405,7 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr cleanup: VIR_FREE(keypath); + VIR_FORCE_FCLOSE(fp); return rc; } -- 1.7.4.4
On 05/03/2011 05:49 PM, Eric Blake wrote:
Clang detected a dead store to rc. It turns out that in fixing this, I also found a FILE* leak.
* src/util/cgroup.c (virCgroupKillInternal): Abort rather than resuming loop on fscanf failure, and cleanup file on error.
This definitely fixes the FILE* leak, but do we really want to abort the loop (stop looking for more pidfiles) when fscanf fails on one pidfile? (dunno, just asking)
---
As a bonus, this also fixes a decl-after-statement for fp.
src/util/cgroup.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c index afe8731..62b371d 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1351,7 +1351,9 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr int killedAny = 0; char *keypath = NULL; bool done = false; - VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); + FILE *fp = NULL; + VIR_DEBUG("group=%p path=%s signum=%d pids=%p", + group, group->path, signum, pids);
rc = virCgroupPathOfController(group, -1, "tasks",&keypath); if (rc != 0) { @@ -1364,7 +1366,6 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr */ while (!done) { done = true; - FILE *fp; if (!(fp = fopen(keypath, "r"))) { rc = -errno; VIR_DEBUG("Failed to read %s: %m\n", keypath); @@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr if (feof(fp)) break; rc = -errno; - break; + VIR_DEBUG("Failed to read %s: %m\n", keypath); + goto cleanup; } if (virHashLookup(pids, (void*)pid)) continue; @@ -1403,6 +1405,7 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr
cleanup: VIR_FREE(keypath); + VIR_FORCE_FCLOSE(fp);
return rc; }
On Wed, May 04, 2011 at 12:30:07AM -0400, Laine Stump wrote:
On 05/03/2011 05:49 PM, Eric Blake wrote:
Clang detected a dead store to rc. It turns out that in fixing this, I also found a FILE* leak.
* src/util/cgroup.c (virCgroupKillInternal): Abort rather than resuming loop on fscanf failure, and cleanup file on error.
This definitely fixes the FILE* leak, but do we really want to abort the loop (stop looking for more pidfiles) when fscanf fails on one pidfile? (dunno, just asking) [...]
@@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr if (feof(fp)) break; rc = -errno; - break; + VIR_DEBUG("Failed to read %s: %m\n", keypath); + goto cleanup; } if (virHashLookup(pids, (void*)pid)) continue;
Seems to me we were doing a break anyway, so it should not change behaviour there, ACK, but let's have a second patch for after 0.9.1 if we think this need improvement. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 05/04/2011 03:35 AM, Daniel Veillard wrote:
On Wed, May 04, 2011 at 12:30:07AM -0400, Laine Stump wrote:
Clang detected a dead store to rc. It turns out that in fixing this, I also found a FILE* leak.
* src/util/cgroup.c (virCgroupKillInternal): Abort rather than resuming loop on fscanf failure, and cleanup file on error. This definitely fixes the FILE* leak, but do we really want to abort
On 05/03/2011 05:49 PM, Eric Blake wrote: the loop (stop looking for more pidfiles) when fscanf fails on one pidfile? (dunno, just asking) [...]
@@ -1376,7 +1377,8 @@ static int virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr if (feof(fp)) break; rc = -errno; - break; + VIR_DEBUG("Failed to read %s: %m\n", keypath); + goto cleanup; } if (virHashLookup(pids, (void*)pid)) continue; Seems to me we were doing a break anyway, so it should not change behaviour there,
The break will get you out of the inner while (!feof(fp)) loop, but if you managed to get through that inner loop once before failing, and set done = false, you will then loop again through while (!done) and go to the next file (if one exists). If you change the break into "goto cleanup", you will skip any remaining files. I don't know if that's important or not, but it is a change in behavior.
ACK, but let's have a second patch for after 0.9.1 if we think this need improvement.
Daniel
On 05/04/2011 06:23 AM, Laine Stump wrote:
This definitely fixes the FILE* leak, but do we really want to abort the loop (stop looking for more pidfiles) when fscanf fails on one pidfile? (dunno, just asking)
Personally, I think that since pidfiles are always generated by the kernel, they should always have sane contents, so failure to read them means something is really wrong. The fact that the overall function returns -1, right away, rather than trying to read other pid files (if any), is in some regards a feature (if we're having problems reading one pid file, then what else is going wrong?). Furthermore, there were other places in the loop that aborted on the first failure, so this isn't the first case of aborting the outer loop if the inner loop fails.
I don't know if that's important or not, but it is a change in behavior.
I've updated the commit message to call attention to the subtle (and unlikely) change in behavior.
ACK, but let's have a second patch for after 0.9.1 if we think this need improvement.
I've pushed the patch unchanged for 0.9.1; right now, I don't think we need any post-0.9.1 improvement, but I'm still open to anyone else that can argue a case for trying to read remaining pid files even if we failed to read one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/04/2011 10:40 AM, Eric Blake wrote:
This definitely fixes the FILE* leak, but do we really want to abort the loop (stop looking for more pidfiles) when fscanf fails on one pidfile? (dunno, just asking) Personally, I think that since pidfiles are always generated by the kernel, they should always have sane contents, so failure to read them means something is really wrong. The fact that the overall function returns -1, right away, rather than trying to read other pid files (if any), is in some regards a feature (if we're having problems reading one
On 05/04/2011 06:23 AM, Laine Stump wrote: pid file, then what else is going wrong?). Furthermore, there were other places in the loop that aborted on the first failure, so this isn't the first case of aborting the outer loop if the inner loop fails.
Fair enough.
I've pushed the patch unchanged for 0.9.1; right now, I don't think we need any post-0.9.1 improvement, but I'm still open to anyone else that can argue a case for trying to read remaining pid files even if we failed to read one.
Nah, not from me. I don't have an opinion on best practice in this case, just wanted to make sure the change was recognized and considered by anyone who might.
participants (3)
-
Daniel Veillard -
Eric Blake -
Laine Stump