Libguestfs - Aug 2009 - [PATCH libguestfs] fish: don't read freed memory

Using the latest code, I was seeing a failure of the remote alloc part
of the test-remote.sh test:

i.e., this would fail:

  make check -C regressions TESTS=test-remote.sh

Running valgrind on it, I got this:

  $ libtool --mode=execute valgrind ../fish/guestfish --remote alloc test.img
10M
  ==11953== Memcheck, a memory error detector.
  ==11953== Copyright (C) 2002-2008, and GNU GPL'd, by Julian Seward et al.
  ==11953== Using LibVEX rev 1884, a library for dynamic binary translation.
  ==11953== Copyright (C) 2004-2008, and GNU GPL'd, by OpenWorks LLP.
  ==11953== Using valgrind-3.4.1, a dynamic binary instrumentation framework.
  ==11953== Copyright (C) 2000-2008, and GNU GPL'd, by Julian Seward et al.
  ==11953== For more details, rerun with: -v
  ==11953=  ==11953== Invalid read of size 2
  ==11953==    at 0x3E89C6653D: fflush (in /lib64/libc-2.10.1.so)
  ==11953==    by 0x415E26: rc_remote (rc.c:269)
  ==11953==    by 0x413C53: issue_command (fish.c:779)
  ==11953==    by 0x414F81: main (fish.c:721)
  ==11953==  Address 0x4e602e8 is 0 bytes inside a block of size 568 free'd
  ==11953==    at 0x4A0633D: free (vg_replace_malloc.c:323)
  ==11953==    by 0x3E89C660DC: fclose@@GLIBC_2.2.5 (in /lib64/libc-2.10.1.so)
  ==11953==    by 0x415E10: rc_remote (rc.c:268)
  ==11953==    by 0x413C53: issue_command (fish.c:779)
  ==11953==    by 0x414F81: main (fish.c:721)
  ==11953=  ==11953== Invalid read of size 8
  ==11953==    at 0x3E89C66543: fflush (in /lib64/libc-2.10.1.so)
  ==11953==    by 0x415E26: rc_remote (rc.c:269)
  ==11953==    by 0x413C53: issue_command (fish.c:779)
  ==11953==    by 0x414F81: main (fish.c:721)
  ==11953==  Address 0x4e60370 is 136 bytes inside a block of size 568
free'd
  ==11953==    at 0x4A0633D: free (vg_replace_malloc.c:323)
  ==11953==    by 0x3E89C660DC: fclose@@GLIBC_2.2.5 (in /lib64/libc-2.10.1.so)
  ==11953==    by 0x415E10: rc_remote (rc.c:268)
  ==11953==    by 0x413C53: issue_command (fish.c:779)
  ==11953==    by 0x414F81: main (fish.c:721)
  ==11953=  ... (and many more) ...

The fix is to ensure that we close the file handle *after*,
not before calling xdr_destroy.
>From 99c1ef6b484866ec1e7bbd621096ea1a2294f31e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Wed, 12 Aug 2009 14:44:14 +0200
Subject: [PATCH libguestfs] fish: don't read freed memory

* fish/rc.c (rc_remote): Close file handle only *after* xdr_destroy,
because that latter may flush its file handle (implicated via
xdrstdio_create).
---
 fish/rc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fish/rc.c b/fish/rc.c
index e88716c..57f1c36 100644
--- a/fish/rc.c
+++ b/fish/rc.c
@@ -235,8 +235,8 @@ rc_remote (int pid, const char *cmd, int argc, char *argv[],

   if (!xdr_guestfish_hello (&xdr, &hello)) {
     fprintf (stderr, _("guestfish: protocol error: could not send initial
greeting to server\n"));
-    fclose (fp);
     xdr_destroy (&xdr);
+    fclose (fp);
     return -1;
   }

@@ -249,8 +249,8 @@ rc_remote (int pid, const char *cmd, int argc, char *argv[],
   call.exit_on_error = exit_on_error;
   if (!xdr_guestfish_call (&xdr, &call)) {
     fprintf (stderr, _("guestfish: protocol error: could not send initial
greeting to server\n"));
-    fclose (fp);
     xdr_destroy (&xdr);
+    fclose (fp);
     return -1;
   }
   xdr_destroy (&xdr);
@@ -260,13 +260,13 @@ rc_remote (int pid, const char *cmd, int argc, char
*argv[],

   if (!xdr_guestfish_reply (&xdr, &reply)) {
     fprintf (stderr, _("guestfish: protocol error: could not decode reply
from server\n"));
-    fclose (fp);
     xdr_destroy (&xdr);
+    fclose (fp);
     return -1;
   }

-  fclose (fp);
   xdr_destroy (&xdr);
+  fclose (fp);

   return reply.r;
 }
--
1.6.4.337.g5420e

On Wed, Aug 12, 2009 at 02:45:02PM +0200, Jim Meyering
wrote:
> Using the latest code, I was seeing a failure of the remote alloc part
> of the test-remote.sh test:
> The fix is to ensure that we close the file handle *after*,
> not before calling xdr_destroy.
>      fprintf (stderr, _("guestfish: protocol error: could not send
initial greeting to server\n"));
> -    fclose (fp);
>      xdr_destroy (&xdr);
> +    fclose (fp);
>      return -1;
Ouch, that's a very subtle and non-obvious bug.  Definite ACK.

I wonder where else we have bugs like that ...

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top