[Qemu-devel] [PATCH] ui/vnc: Fix problem with sending too many bytes as server name

[Qemu-devel] [PATCH] ui/vnc: Fix problem with sending too many bytes as server name

[Thomas Huth]

If the buffer is not big enough, snprintf() does not return the number
of bytes that have been written to the buffer, but the number of bytes
that would be needed for writing the whole string. By using this value
for the following vnc_write() calls, we send some junk at the end of
the name in case the qemu_name is longer than 1017 bytes, which could
confuse the VNC clients. Fix this by adding an additional size check
here.

Buglink: https://bugs.launchpad.net/qemu/+bug/1637447
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 ui/vnc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 2c28a59..29aa9c4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2459,10 +2459,14 @@ static int protocol_client_init(VncState *vs, uint8_t *data, size_t len)
 
     pixel_format_message(vs);
 
-    if (qemu_name)
+    if (qemu_name) {
         size = snprintf(buf, sizeof(buf), "QEMU (%s)", qemu_name);
-    else
+        if (size > sizeof(buf)) {
+            size = sizeof(buf);
+        }
+    } else {
         size = snprintf(buf, sizeof(buf), "QEMU");
+    }
 
     vnc_write_u32(vs, size);
     vnc_write(vs, buf, size);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.8] ui/vnc: Fix problem with sending too many bytes as server name

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On 11/21/2016 11:25 AM, Thomas Huth wrote:
> If the buffer is not big enough, snprintf() does not return the number
> of bytes that have been written to the buffer, but the number of bytes
> that would be needed for writing the whole string. By using this value
> for the following vnc_write() calls, we send some junk at the end of
> the name in case the qemu_name is longer than 1017 bytes, which could
> confuse the VNC clients. Fix this by adding an additional size check
> here.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1637447
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  ui/vnc.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Worth having in 2.8, I think.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] ui/vnc: Fix problem with sending too many bytes as server name

[Gerd Hoffmann]

On Mo, 2016-11-21 at 18:25 +0100, Thomas Huth wrote:
> If the buffer is not big enough, snprintf() does not return the number
> of bytes that have been written to the buffer, but the number of bytes
> that would be needed for writing the whole string. By using this value
> for the following vnc_write() calls, we send some junk at the end of
> the name in case the qemu_name is longer than 1017 bytes, which could
> confuse the VNC clients. Fix this by adding an additional size check
> here.

Use g_strdup_printf instead?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ui/vnc: Fix problem with sending too many bytes as server name

[Thomas Huth]

On 21.11.2016 18:51, Gerd Hoffmann wrote:
> On Mo, 2016-11-21 at 18:25 +0100, Thomas Huth wrote:
>> If the buffer is not big enough, snprintf() does not return the number
>> of bytes that have been written to the buffer, but the number of bytes
>> that would be needed for writing the whole string. By using this value
>> for the following vnc_write() calls, we send some junk at the end of
>> the name in case the qemu_name is longer than 1017 bytes, which could
>> confuse the VNC clients. Fix this by adding an additional size check
>> here.
> 
> Use g_strdup_printf instead?

Not sure ... If I'd use g_strdup_printf instead here, we might end up
with a string that is bigger than 1024 bytes here. Maybe there is/was a
reason that the string is limited to 1024 bytes here? Are there clients
that can not deal with more that 1024 bytes?
So unless you feel very confident that it does not matter, using the
size check instead of g_strdup_printf sounds like the less invasive
solution to me.

 Thomas

Re: [Qemu-devel] [PATCH] ui/vnc: Fix problem with sending too many bytes as server name

[Gerd Hoffmann]

On Mo, 2016-11-21 at 18:25 +0100, Thomas Huth wrote:
> If the buffer is not big enough, snprintf() does not return the number
> of bytes that have been written to the buffer, but the number of bytes
> that would be needed for writing the whole string. By using this value
> for the following vnc_write() calls, we send some junk at the end of
> the name in case the qemu_name is longer than 1017 bytes, which could
> confuse the VNC clients. Fix this by adding an additional size check
> here.

Added to ui queue.

thanks,
  Gerd