[Qemu-devel] [PATCH] Fix changing password using monitor over VNC.

[Zachary Amsden]

A simple segfault turned out to be a relatively complex fix.

The monitor calls back to main_loop_wait() to wait for the completion
of the password change event; this results in a nested invocation of
the associated I/O handlers.  For stdio monitor, this is okay, but VNC
maintains an input buffer which is not flushed until after the
invocation of protocol actions.  This is non-reentrant; the result is
that the nested invocation consumes the same protocol event as the
parent (which was a '\n', setting a NULL password), and it gets worse
when both the child and the parent attempt to shift in the same input
event, resulting in a memmove of size -1ULL, and a segfault.

The fix is to consume the input buffer before invoking protocol actions
which may cause nested invocation of the handler; we must also set up
the child handler to receive new events, which was cleanest done with
vnc_read_when() from the protcol handler (doing it in the outer loop
causes bugs with other types of waits, such as auth).  We return fed=1
from the outer handler to prevent the logic in vnc_client_read from
reconsuming the pre-consumed buffer, and simply reset the expect
value to receive the next protocol command.

Signed-off-by: Zachary Amsden <address@hidden>
---
 vnc.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/vnc.c b/vnc.c
index 2b3a6eb..3e88855 100644
--- a/vnc.c
+++ b/vnc.c
@@ -798,6 +798,15 @@ static void vnc_read_when(VncState *vs, VncReadEvent 
*func, size_t expecting)
     vs->read_handler_expect = expecting;
 }
 
+static void vnc_client_consume(VncState *vs, size_t len)
+{
+    if (vs->input.offset >= len) {
+        memmove(vs->input.buffer, vs->input.buffer + len,
+                (vs->input.offset - len));
+        vs->input.offset -= len;
+    }
+}
+
 static void vnc_client_read(void *opaque)
 {
     VncState *vs = opaque;
@@ -833,8 +842,7 @@ static void vnc_client_read(void *opaque)
            return;
 
        if (!ret) {
-           memmove(vs->input.buffer, vs->input.buffer + len, (vs->input.offset 
- len));
-           vs->input.offset -= len;
+            vnc_client_consume(vs, len);
        } else {
            vs->read_handler_expect = ret;
        }
@@ -1357,6 +1365,7 @@ static int protocol_client_msg(VncState *vs, uint8_t 
*data, size_t len)
 {
     int i;
     uint16_t limit;
+    int fed = 0;
 
     switch (data[0]) {
     case 0:
@@ -1399,6 +1408,9 @@ static int protocol_client_msg(VncState *vs, uint8_t 
*data, size_t len)
        if (len == 1)
            return 8;
 
+        vnc_client_consume(vs, len);
+        vnc_read_when(vs, protocol_client_msg, 1);
+        fed = 1;
        key_event(vs, read_u8(data, 1), read_u32(data, 4));
        break;
     case 5:
@@ -1428,6 +1440,9 @@ static int protocol_client_msg(VncState *vs, uint8_t 
*data, size_t len)
             if (len == 2)
                 return 12;
 
+            vnc_client_consume(vs, len);
+            vnc_read_when(vs, protocol_client_msg, 1);
+            fed = 1;
             ext_key_event(vs, read_u16(data, 2),
                           read_u32(data, 4), read_u32(data, 8));
             break;
@@ -1486,7 +1501,7 @@ static int protocol_client_msg(VncState *vs, uint8_t 
*data, size_t len)
     }
 
     vnc_read_when(vs, protocol_client_msg, 1);
-    return 0;
+    return fed;
 }
 
 static int protocol_client_init(VncState *vs, uint8_t *data, size_t len)
@@ -2285,6 +2300,8 @@ int vnc_display_password(DisplayState *ds, const char 
*password)
        if (!(vs->password = qemu_strdup(password)))
            return -1;
     }
+    if (vs->auth == VNC_AUTH_NONE)
+        vs->auth = VNC_AUTH_VNC;
 
     return 0;
 }
-- 
1.6.2.2.471.g6da14