[libvirt] [PATCH] Fix race condition in qemuGetProcessInfo

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141201/4e7282d0/attachment-0001.sig>