Libguestfs - Dec 2014 - [PATCH] p2v: avoid connecting to ourself while probing qemu-nbd (RHBZ#1167774)

---
 p2v/conversion.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/p2v/conversion.c b/p2v/conversion.c
index 4ff7ecc..5c0c78e 100644
--- a/p2v/conversion.c
+++ b/p2v/conversion.c
@@ -44,6 +44,16 @@
 /* How long to wait for qemu-nbd to start (seconds). */
 #define WAIT_QEMU_NBD_TIMEOUT 10
 
+/* Source port for probing qemu-nbd. Should be one greater than
+ * nbd_local_port in ssh.c (50123).  It's not guaranteed to always
+ * bind to 50124, but it will hint the kernel to start there and try
+ * incrementally higher ports if needed.  This avoids the case where
+ * the kernel selects 50123 as our source port, and we immediately
+ * connect to ourself.
+ * See: https://bugzilla.redhat.com/show_bug.cgi?id=1167774#c9
+ */
+static int nbd_probe_port = 50124;
+
 /* Data per NBD connection / physical disk. */
 struct data_conn {
   mexp_h *h;                /* miniexpect handle to ssh */
@@ -398,7 +408,7 @@ wait_qemu_nbd (int nbd_local_port, int timeout_seconds)
 {
   int sockfd;
   int result = -1;
-  struct sockaddr_in addr;
+  struct sockaddr_in src_addr, dst_addr;
   time_t start_t, now_t;
   struct timeval timeout = { .tv_usec = 0 };
   char magic[8]; /* NBDMAGIC */
@@ -413,10 +423,20 @@ wait_qemu_nbd (int nbd_local_port, int timeout_seconds)
     return -1;
   }
 
-  memset (&addr, 0, sizeof addr);
-  addr.sin_family = AF_INET;
-  addr.sin_port = htons (nbd_local_port);
-  inet_pton (AF_INET, "localhost", &addr.sin_addr);
+  memset (&src_addr, 0, sizeof src_addr);
+  src_addr.sin_family = AF_INET;
+  src_addr.sin_port = htons (nbd_probe_port);
+  inet_pton (AF_INET, "localhost", &src_addr.sin_addr);
+
+  memset (&dst_addr, 0, sizeof dst_addr);
+  dst_addr.sin_family = AF_INET;
+  dst_addr.sin_port = htons (nbd_local_port);
+  inet_pton (AF_INET, "localhost", &dst_addr.sin_addr);
+
+  if (bind (sockfd, (struct sockaddr *) &src_addr, sizeof src_addr) == -1)
{
+    perror ("bind");
+    return -1;
+  }
 
   for (;;) {
     time (&now_t);
@@ -426,7 +446,7 @@ wait_qemu_nbd (int nbd_local_port, int timeout_seconds)
       goto cleanup;
     }
 
-    if (connect (sockfd, (struct sockaddr *) &addr, sizeof addr) == 0)
+    if (connect (sockfd, (struct sockaddr *) &dst_addr, sizeof dst_addr) ==
0)
       break;
   }
 
-- 
2.1.0

On Fri, Dec 12, 2014 at 12:10:06PM -0500, John Eckersberg
wrote:
> +  if (bind (sockfd, (struct sockaddr *) &src_addr, sizeof src_addr) ==
-1) {
> +    perror ("bind");
This line needs to be:

    set_conversion_error ("waiting for qemu-nbd to start: bind: %m");

I'm trying to make the test hard to see if I can break this one too :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

The attached patch is supposed to make the test a bit meaner ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top