2:24 p.m.
There is a race condition between the fopen and fscanf calls
in qemuGetProcessInfo. If fopen succeeds, there is a small
possibility that the file no longer exists before reading from it.
Now, if either fopen or fscanf calls fail, the function will behave
just as only fopen had failed.
---
src/qemu/qemu_driver.c | 43 ++++++++++++++++---------------------------
1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ad5f70a..9ab1a81 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1310,9 +1310,9 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
*vm_rss,
{
char *proc;
FILE *pidinfo;
- unsigned long long usertime, systime;
- long rss;
- int cpu;
+ unsigned long long usertime = 0, systime = 0;
+ long rss = 0;
+ int cpu = 0;
int ret;
/* In general, we cannot assume pid_t fits in int; but /proc parsing
@@ -1324,33 +1324,24 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
*vm_rss,
if (ret < 0)
return -1;
- if (!(pidinfo = fopen(proc, "r"))) {
- /* VM probably shut down, so fake 0 */
- if (cpuTime)
- *cpuTime = 0;
- if (lastCpu)
- *lastCpu = 0;
- if (vm_rss)
- *vm_rss = 0;
- VIR_FREE(proc);
- return 0;
- }
+ pidinfo = fopen(proc, "r");
VIR_FREE(proc);
/* See 'man proc' for information about what all these fields are. We're
* only interested in a very few of them */
- if (fscanf(pidinfo,
- /* pid -> stime */
- "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu"
- /* cutime -> endcode */
- "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
- /* startstack -> processor */
- "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
- &usertime, &systime, &rss, &cpu) != 4) {
+ if (pidinfo) {
+ if (fscanf(pidinfo,
+ /* pid -> stime */
+ "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu
%llu"
+ /* cutime -> endcode */
+ "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
+ /* startstack -> processor */
+ "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
+ &usertime, &systime, &rss, &cpu) != 4) {
+ VIR_WARN("cannot parse process status data");
+ usertime = systime = 0; rss = 0; cpu = 0;
+ }
VIR_FORCE_FCLOSE(pidinfo);
- VIR_WARN("cannot parse process status data");
- errno = -EINVAL;
- return -1;
}
/* We got jiffies
@@ -1374,8 +1365,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
*vm_rss,
VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
(int) pid, tid, usertime, systime, cpu, rss);
- VIR_FORCE_FCLOSE(pidinfo);
-
return 0;
}
--
1.7.9.5
4:43 p.m.
On 12/01/2014 01:24 PM, Eduardo Costa wrote:
Thanks for forwarding to the list. I'll modify the commit message to
include a reference to
https://bugzilla.redhat.com/show_bug.cgi?id=1169055 where you first
posted this.
There is a race condition between the fopen and fscanf calls
in qemuGetProcessInfo. If fopen succeeds, there is a small
possibility that the file no longer exists before reading from it.
Now, if either fopen or fscanf calls fail, the function will behave
just as only fopen had failed.
---
src/qemu/qemu_driver.c | 43 ++++++++++++++++---------------------------
1 file changed, 16 insertions(+), 27 deletions(-)
/* See 'man proc' for information about what all these fields are.
We're
* only interested in a very few of them */
- if (fscanf(pidinfo,
- /* pid -> stime */
Rather than reindenting all this,...
- "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u
%*u %llu %llu"
- /* cutime -> endcode */
- "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
- /* startstack -> processor */
- "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
- &usertime, &systime, &rss, &cpu) != 4) {
+ if (pidinfo) {
+ if (fscanf(pidinfo,
...why not just write:
if (!pidinfo ||
fscanf(...)
+ /* pid -> stime */
+ "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu
%llu"
+ /* cutime -> endcode */
+ "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
+ /* startstack -> processor */
+ "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
+ &usertime, &systime, &rss, &cpu) != 4) {
+ VIR_WARN("cannot parse process status data");
+ usertime = systime = 0; rss = 0; cpu = 0;
Why explicitly set things back to 0? You already initialized them to
sane values, and I'd rather use partial population than nothing at all
(in all likelihood, since fscanf is buffered, you either got all-or-none
on a single underlying read() from the file descriptor, depending on
whether the race caused the read to fail because the process has died in
the meantime).
+ }
VIR_FORCE_FCLOSE(pidinfo);
- VIR_WARN("cannot parse process status data");
- errno = -EINVAL;
- return -1;
}
But this part I can agree with.
/* We got jiffies
@@ -1374,8 +1365,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
*vm_rss,
VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
(int) pid, tid, usertime, systime, cpu, rss);
- VIR_FORCE_FCLOSE(pidinfo);
-
If you simplify the above 'if' to avoid reindentation, then this is
still necessary. Here's the simplified patch I'm pushing in your name;
congratulations on your first libvirt patch! (Oh, and I reformatted a
long line in the process of touching this function...)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a956c59..ec93191 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1310,9 +1310,9 @@ qemuGetProcessInfo(unsigned long long *cpuTime,
int *lastCpu, long *vm_rss,
{
char *proc;
FILE *pidinfo;
- unsigned long long usertime, systime;
- long rss;
- int cpu;
+ unsigned long long usertime = 0, systime = 0;
+ long rss = 0;
+ int cpu = 0;
int ret;
/* In general, we cannot assume pid_t fits in int; but /proc parsing
@@ -1324,22 +1324,13 @@ qemuGetProcessInfo(unsigned long long *cpuTime,
int *lastCpu, long *vm_rss,
if (ret < 0)
return -1;
- if (!(pidinfo = fopen(proc, "r"))) {
- /* VM probably shut down, so fake 0 */
- if (cpuTime)
- *cpuTime = 0;
- if (lastCpu)
- *lastCpu = 0;
- if (vm_rss)
- *vm_rss = 0;
- VIR_FREE(proc);
- return 0;
- }
+ pidinfo = fopen(proc, "r");
VIR_FREE(proc);
/* See 'man proc' for information about what all these fields are.
We're
* only interested in a very few of them */
- if (fscanf(pidinfo,
+ if (!pidinfo ||
+ fscanf(pidinfo,
/* pid -> stime */
"%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u
%llu %llu"
/* cutime -> endcode */
@@ -1347,10 +1338,7 @@ qemuGetProcessInfo(unsigned long long *cpuTime,
int *lastCpu, long *vm_rss,
/* startstack -> processor */
"%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
&usertime, &systime, &rss, &cpu) != 4) {
- VIR_FORCE_FCLOSE(pidinfo);
VIR_WARN("cannot parse process status data");
- errno = -EINVAL;
- return -1;
}
/* We got jiffies
@@ -1359,7 +1347,8 @@ qemuGetProcessInfo(unsigned long long *cpuTime,
int *lastCpu, long *vm_rss,
* So calculate thus....
*/
if (cpuTime)
- *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) /
(unsigned long long)sysconf(_SC_CLK_TCK);
+ *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime)
+ / (unsigned long long)sysconf(_SC_CLK_TCK);
if (lastCpu)
*lastCpu = cpu;
--
1.9.3
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3644
days inactive
3644
days old
1 comments
2 participants
participants (2)
-
Eduardo Costa -
Eric Blake