[PATCH 0/2] Lost VNC patches

[PATCH 0/2] Lost VNC patches

[Corentin Chary]

Hi Anthony,

Please merge these two patchs from another age, they fix crash in the VNC
server (the iohandler one is only for the threaded server).

Thanks,

Corentin Chary (1):
  vnc: don't mess up with iohandlers in the vnc thread

Stefan Weil (1):
  vnc: Limit r/w access to size of allocated memory

 ui/vnc-jobs-async.c |   48 +++++++++++++++++++++++++++++-------------------
 ui/vnc-jobs.h       |    1 +
 ui/vnc.c            |   15 +++++++++++++++
 ui/vnc.h            |    2 ++
 4 files changed, 47 insertions(+), 19 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 0/2] Lost VNC patches

[Corentin Chary]

Hi Anthony,

Please merge these two patchs from another age, they fix crash in the VNC
server (the iohandler one is only for the threaded server).

Thanks,

Corentin Chary (1):
  vnc: don't mess up with iohandlers in the vnc thread

Stefan Weil (1):
  vnc: Limit r/w access to size of allocated memory

 ui/vnc-jobs-async.c |   48 +++++++++++++++++++++++++++++-------------------
 ui/vnc-jobs.h       |    1 +
 ui/vnc.c            |   15 +++++++++++++++
 ui/vnc.h            |    2 ++
 4 files changed, 47 insertions(+), 19 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] vnc: don't mess up with iohandlers in the vnc thread

[Corentin Chary]

From: Corentin Chary <corentin.chary@gmail.com>

The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

Using qemu_mutex_lock_iothread() won't work because vnc_dpy_cpy(),
which will wait for the current job queue to finish, can be called with
the iothread lock held.

Instead, we now store the data in a temporary buffer, and use a bottom
half to notify the main thread that new data is available.

vnc_[un]lock_ouput() is still needed to access VncState members like
abort, csock or jobs_buffer.

Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
---
 ui/vnc-jobs-async.c |   48 +++++++++++++++++++++++++++++-------------------
 ui/vnc-jobs.h       |    1 +
 ui/vnc.c            |   12 ++++++++++++
 ui/vnc.h            |    2 ++
 4 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 9b3016c..087b84d 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -28,6 +28,7 @@
 
 #include "vnc.h"
 #include "vnc-jobs.h"
+#include "qemu_socket.h"
 
 /*
  * Locking:
@@ -155,6 +156,24 @@ void vnc_jobs_join(VncState *vs)
         qemu_cond_wait(&queue->cond, &queue->mutex);
     }
     vnc_unlock_queue(queue);
+    vnc_jobs_consume_buffer(vs);
+}
+
+void vnc_jobs_consume_buffer(VncState *vs)
+{
+    bool flush;
+
+    vnc_lock_output(vs);
+    if (vs->jobs_buffer.offset) {
+        vnc_write(vs, vs->jobs_buffer.buffer, vs->jobs_buffer.offset);
+        buffer_reset(&vs->jobs_buffer);
+    }
+    flush = vs->csock != -1 && vs->abort != true;
+    vnc_unlock_output(vs);
+
+    if (flush) {
+      vnc_flush(vs);
+    }
 }
 
 /*
@@ -197,7 +216,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     VncState vs;
     int n_rectangles;
     int saved_offset;
-    bool flush;
 
     vnc_lock_queue(queue);
     while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) {
@@ -213,6 +231,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
     vnc_lock_output(job->vs);
     if (job->vs->csock == -1 || job->vs->abort == true) {
+        vnc_unlock_output(job->vs);
         goto disconnected;
     }
     vnc_unlock_output(job->vs);
@@ -233,10 +252,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
         if (job->vs->csock == -1) {
             vnc_unlock_display(job->vs->vd);
-            /* output mutex must be locked before going to
-             * disconnected:
-             */
-            vnc_lock_output(job->vs);
             goto disconnected;
         }
 
@@ -254,24 +269,19 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     vs.output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
     vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
 
-    /* Switch back buffers */
     vnc_lock_output(job->vs);
-    if (job->vs->csock == -1) {
-        goto disconnected;
+    if (job->vs->csock != -1) {
+        buffer_reserve(&job->vs->jobs_buffer, vs.output.offset);
+        buffer_append(&job->vs->jobs_buffer, vs.output.buffer,
+                      vs.output.offset);
+        /* Copy persistent encoding data */
+        vnc_async_encoding_end(job->vs, &vs);
+
+	qemu_bh_schedule(job->vs->bh);
     }
-
-    vnc_write(job->vs, vs.output.buffer, vs.output.offset);
-
-disconnected:
-    /* Copy persistent encoding data */
-    vnc_async_encoding_end(job->vs, &vs);
-    flush = (job->vs->csock != -1 && job->vs->abort != true);
     vnc_unlock_output(job->vs);
 
-    if (flush) {
-        vnc_flush(job->vs);
-    }
-
+disconnected:
     vnc_lock_queue(queue);
     QTAILQ_REMOVE(&queue->jobs, job, next);
     vnc_unlock_queue(queue);
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index b8dab81..4c661f9 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -40,6 +40,7 @@ void vnc_jobs_join(VncState *vs);
 
 #ifdef CONFIG_VNC_THREAD
 
+void vnc_jobs_consume_buffer(VncState *vs);
 void vnc_start_worker_thread(void);
 bool vnc_worker_thread_running(void);
 void vnc_stop_worker_thread(void);
diff --git a/ui/vnc.c b/ui/vnc.c
index bdec33a..aef6d3a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1068,7 +1068,10 @@ static void vnc_disconnect_finish(VncState *vs)
 
 #ifdef CONFIG_VNC_THREAD
     qemu_mutex_destroy(&vs->output_mutex);
+    qemu_bh_delete(vs->bh);
+    buffer_free(&vs->jobs_buffer);
 #endif
+
     for (i = 0; i < VNC_STAT_ROWS; ++i) {
         g_free(vs->lossy_rect[i]);
     }
@@ -1283,6 +1286,14 @@ static long vnc_client_read_plain(VncState *vs)
     return ret;
 }
 
+#ifdef CONFIG_VNC_THREAD
+static void vnc_jobs_bh(void *opaque)
+{
+    VncState *vs = opaque;
+
+    vnc_jobs_consume_buffer(vs);
+}
+#endif
 
 /*
  * First function called whenever there is more data to be read from
@@ -2687,6 +2698,7 @@ static void vnc_connect(VncDisplay *vd, int csock, int skipauth)
 
 #ifdef CONFIG_VNC_THREAD
     qemu_mutex_init(&vs->output_mutex);
+    vs->bh = qemu_bh_new(vnc_jobs_bh, vs);
 #endif
 
     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
diff --git a/ui/vnc.h b/ui/vnc.h
index 0bd1fc6..a851ebd 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -304,6 +304,8 @@ struct VncState
     VncJob job;
 #else
     QemuMutex output_mutex;
+    QEMUBH *bh;
+    Buffer jobs_buffer;
 #endif
 
     /* Encoding specific, if you add something here, don't forget to
-- 
1.7.3.4

[Qemu-devel] [PATCH 1/2] vnc: don't mess up with iohandlers in the vnc thread

[Corentin Chary]

From: Corentin Chary <corentin.chary@gmail.com>

The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

Using qemu_mutex_lock_iothread() won't work because vnc_dpy_cpy(),
which will wait for the current job queue to finish, can be called with
the iothread lock held.

Instead, we now store the data in a temporary buffer, and use a bottom
half to notify the main thread that new data is available.

vnc_[un]lock_ouput() is still needed to access VncState members like
abort, csock or jobs_buffer.

Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
---
 ui/vnc-jobs-async.c |   48 +++++++++++++++++++++++++++++-------------------
 ui/vnc-jobs.h       |    1 +
 ui/vnc.c            |   12 ++++++++++++
 ui/vnc.h            |    2 ++
 4 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 9b3016c..087b84d 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -28,6 +28,7 @@
 
 #include "vnc.h"
 #include "vnc-jobs.h"
+#include "qemu_socket.h"
 
 /*
  * Locking:
@@ -155,6 +156,24 @@ void vnc_jobs_join(VncState *vs)
         qemu_cond_wait(&queue->cond, &queue->mutex);
     }
     vnc_unlock_queue(queue);
+    vnc_jobs_consume_buffer(vs);
+}
+
+void vnc_jobs_consume_buffer(VncState *vs)
+{
+    bool flush;
+
+    vnc_lock_output(vs);
+    if (vs->jobs_buffer.offset) {
+        vnc_write(vs, vs->jobs_buffer.buffer, vs->jobs_buffer.offset);
+        buffer_reset(&vs->jobs_buffer);
+    }
+    flush = vs->csock != -1 && vs->abort != true;
+    vnc_unlock_output(vs);
+
+    if (flush) {
+      vnc_flush(vs);
+    }
 }
 
 /*
@@ -197,7 +216,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     VncState vs;
     int n_rectangles;
     int saved_offset;
-    bool flush;
 
     vnc_lock_queue(queue);
     while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) {
@@ -213,6 +231,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
     vnc_lock_output(job->vs);
     if (job->vs->csock == -1 || job->vs->abort == true) {
+        vnc_unlock_output(job->vs);
         goto disconnected;
     }
     vnc_unlock_output(job->vs);
@@ -233,10 +252,6 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
         if (job->vs->csock == -1) {
             vnc_unlock_display(job->vs->vd);
-            /* output mutex must be locked before going to
-             * disconnected:
-             */
-            vnc_lock_output(job->vs);
             goto disconnected;
         }
 
@@ -254,24 +269,19 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
     vs.output.buffer[saved_offset] = (n_rectangles >> 8) & 0xFF;
     vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
 
-    /* Switch back buffers */
     vnc_lock_output(job->vs);
-    if (job->vs->csock == -1) {
-        goto disconnected;
+    if (job->vs->csock != -1) {
+        buffer_reserve(&job->vs->jobs_buffer, vs.output.offset);
+        buffer_append(&job->vs->jobs_buffer, vs.output.buffer,
+                      vs.output.offset);
+        /* Copy persistent encoding data */
+        vnc_async_encoding_end(job->vs, &vs);
+
+	qemu_bh_schedule(job->vs->bh);
     }
-
-    vnc_write(job->vs, vs.output.buffer, vs.output.offset);
-
-disconnected:
-    /* Copy persistent encoding data */
-    vnc_async_encoding_end(job->vs, &vs);
-    flush = (job->vs->csock != -1 && job->vs->abort != true);
     vnc_unlock_output(job->vs);
 
-    if (flush) {
-        vnc_flush(job->vs);
-    }
-
+disconnected:
     vnc_lock_queue(queue);
     QTAILQ_REMOVE(&queue->jobs, job, next);
     vnc_unlock_queue(queue);
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index b8dab81..4c661f9 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -40,6 +40,7 @@ void vnc_jobs_join(VncState *vs);
 
 #ifdef CONFIG_VNC_THREAD
 
+void vnc_jobs_consume_buffer(VncState *vs);
 void vnc_start_worker_thread(void);
 bool vnc_worker_thread_running(void);
 void vnc_stop_worker_thread(void);
diff --git a/ui/vnc.c b/ui/vnc.c
index bdec33a..aef6d3a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1068,7 +1068,10 @@ static void vnc_disconnect_finish(VncState *vs)
 
 #ifdef CONFIG_VNC_THREAD
     qemu_mutex_destroy(&vs->output_mutex);
+    qemu_bh_delete(vs->bh);
+    buffer_free(&vs->jobs_buffer);
 #endif
+
     for (i = 0; i < VNC_STAT_ROWS; ++i) {
         g_free(vs->lossy_rect[i]);
     }
@@ -1283,6 +1286,14 @@ static long vnc_client_read_plain(VncState *vs)
     return ret;
 }
 
+#ifdef CONFIG_VNC_THREAD
+static void vnc_jobs_bh(void *opaque)
+{
+    VncState *vs = opaque;
+
+    vnc_jobs_consume_buffer(vs);
+}
+#endif
 
 /*
  * First function called whenever there is more data to be read from
@@ -2687,6 +2698,7 @@ static void vnc_connect(VncDisplay *vd, int csock, int skipauth)
 
 #ifdef CONFIG_VNC_THREAD
     qemu_mutex_init(&vs->output_mutex);
+    vs->bh = qemu_bh_new(vnc_jobs_bh, vs);
 #endif
 
     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
diff --git a/ui/vnc.h b/ui/vnc.h
index 0bd1fc6..a851ebd 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -304,6 +304,8 @@ struct VncState
     VncJob job;
 #else
     QemuMutex output_mutex;
+    QEMUBH *bh;
+    Buffer jobs_buffer;
 #endif
 
     /* Encoding specific, if you add something here, don't forget to
-- 
1.7.3.4

[PATCH 2/2] vnc: Limit r/w access to size of allocated memory

[Corentin Chary]

From: Stefan Weil <weil@mail.berlios.de>

This fixes memory reads and writes which exceeded the upper limit
of allocated memory vd->guest.ds->data and vd->server->data.

Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 ui/vnc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index aef6d3a..deb9ecd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2562,6 +2562,9 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      * Update server dirty map.
      */
     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
+    if (cmp_bytes > vd->ds->surface->linesize) {
+        cmp_bytes = vd->ds->surface->linesize;
+    }
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;
     for (y = 0; y < vd->guest.ds->height; y++) {
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/2] vnc: Limit r/w access to size of allocated memory

[Corentin Chary]

From: Stefan Weil <weil@mail.berlios.de>

This fixes memory reads and writes which exceeded the upper limit
of allocated memory vd->guest.ds->data and vd->server->data.

Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 ui/vnc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index aef6d3a..deb9ecd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2562,6 +2562,9 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      * Update server dirty map.
      */
     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
+    if (cmp_bytes > vd->ds->surface->linesize) {
+        cmp_bytes = vd->ds->surface->linesize;
+    }
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;
     for (y = 0; y < vd->guest.ds->height; y++) {
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 0/2] Lost VNC patches

[Anthony Liguori]

On 03/14/2012 01:58 AM, Corentin Chary wrote:
> Hi Anthony,
>
> Please merge these two patchs from another age, they fix crash in the VNC
> server (the iohandler one is only for the threaded server).

Applied.  Thanks.

Regards,

Anthony Liguori

>
> Thanks,
>
> Corentin Chary (1):
>    vnc: don't mess up with iohandlers in the vnc thread
>
> Stefan Weil (1):
>    vnc: Limit r/w access to size of allocated memory
>
>   ui/vnc-jobs-async.c |   48 +++++++++++++++++++++++++++++-------------------
>   ui/vnc-jobs.h       |    1 +
>   ui/vnc.c            |   15 +++++++++++++++
>   ui/vnc.h            |    2 ++
>   4 files changed, 47 insertions(+), 19 deletions(-)
>

Re: [PATCH 0/2] Lost VNC patches

[Stefan Weil]

Am 14.03.2012 22:46, schrieb Anthony Liguori:
> On 03/14/2012 01:58 AM, Corentin Chary wrote:
>> Hi Anthony,
>>
>> Please merge these two patchs from another age, they fix crash in the 
>> VNC
>> server (the iohandler one is only for the threaded server).
>
> Applied.  Thanks.
>
> Regards,
>
> Anthony Liguori
>

The commit time of my patch was modified here. I had sent the patch on
March 15, 2011, 6:45 p.m., so you could also have waited a day longer
until its first birthday :-)

There is a more serious background why I write this mail: commit
requests should not modify the time when a patch was written.

See http://patchwork.ozlabs.org/patch/87029/ for the original.

Cheers,
Stefan W.



>>
>> Thanks,
>>
>> Corentin Chary (1):
>>    vnc: don't mess up with iohandlers in the vnc thread
>>
>> Stefan Weil (1):
>>    vnc: Limit r/w access to size of allocated memory
>>
>>   ui/vnc-jobs-async.c |   48 
>> +++++++++++++++++++++++++++++-------------------
>>   ui/vnc-jobs.h       |    1 +
>>   ui/vnc.c            |   15 +++++++++++++++
>>   ui/vnc.h            |    2 ++
>>   4 files changed, 47 insertions(+), 19 deletions(-)
>>
>

Re: [Qemu-devel] [PATCH 0/2] Lost VNC patches

[Stefan Weil]

Am 14.03.2012 22:46, schrieb Anthony Liguori:
> On 03/14/2012 01:58 AM, Corentin Chary wrote:
>> Hi Anthony,
>>
>> Please merge these two patchs from another age, they fix crash in the 
>> VNC
>> server (the iohandler one is only for the threaded server).
>
> Applied.  Thanks.
>
> Regards,
>
> Anthony Liguori
>

The commit time of my patch was modified here. I had sent the patch on
March 15, 2011, 6:45 p.m., so you could also have waited a day longer
until its first birthday :-)

There is a more serious background why I write this mail: commit
requests should not modify the time when a patch was written.

See http://patchwork.ozlabs.org/patch/87029/ for the original.

Cheers,
Stefan W.



>>
>> Thanks,
>>
>> Corentin Chary (1):
>>    vnc: don't mess up with iohandlers in the vnc thread
>>
>> Stefan Weil (1):
>>    vnc: Limit r/w access to size of allocated memory
>>
>>   ui/vnc-jobs-async.c |   48 
>> +++++++++++++++++++++++++++++-------------------
>>   ui/vnc-jobs.h       |    1 +
>>   ui/vnc.c            |   15 +++++++++++++++
>>   ui/vnc.h            |    2 ++
>>   4 files changed, 47 insertions(+), 19 deletions(-)
>>
>

Re: [Qemu-devel] [PATCH 0/2] Lost VNC patches

[Corentin Chary]

On Wed, Mar 14, 2012 at 11:16 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 14.03.2012 22:46, schrieb Anthony Liguori:
>
>> On 03/14/2012 01:58 AM, Corentin Chary wrote:
>>>
>>> Hi Anthony,
>>>
>>> Please merge these two patchs from another age, they fix crash in the VNC
>>> server (the iohandler one is only for the threaded server).
>>
>>
>> Applied.  Thanks.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>
> The commit time of my patch was modified here. I had sent the patch on
> March 15, 2011, 6:45 p.m., so you could also have waited a day longer
> until its first birthday :-)
>
> There is a more serious background why I write this mail: commit
> requests should not modify the time when a patch was written.
>
> See http://patchwork.ozlabs.org/patch/87029/ for the original.

Hello Stefan,

Sorry for that, but date was correct on the patch I've sent, it seems
that some smtp server (or the list ?) took the liberty to change it.
I should have sent a pull link instead.

Thanks,

-- 
Corentin Chary
http://xf.iksaif.net