[Qemu-devel] [PATCH] io: Always remove an old channel watch before adding a new one.

[Qemu-devel] [PATCH] io: Always remove an old channel watch before adding a new one.

[Brandon Carpenter]

Also set saved handle to zero when removing without adding a new watch.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 ui/vnc-auth-vencrypt.c | 3 +++
 ui/vnc-ws.c            | 6 ++++++
 ui/vnc.c               | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
index ffaab57550..c3eece4fa7 100644
--- a/ui/vnc-auth-vencrypt.c
+++ b/ui/vnc-auth-vencrypt.c
@@ -77,6 +77,9 @@ static void vnc_tls_handshake_done(QIOTask *task,
         vnc_client_error(vs);
         error_free(err);
     } else {
+        if (vs->ioc_tag) {
+            g_source_remove(vs->ioc_tag);
+        }
         vs->ioc_tag = qio_channel_add_watch(
             vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
         start_auth_vencrypt_subauth(vs);
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index f530cd5474..eaf309553c 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -36,6 +36,9 @@ static void vncws_tls_handshake_done(QIOTask *task,
         error_free(err);
     } else {
         VNC_DEBUG("TLS handshake complete, starting websocket handshake\n");
+        if (vs->ioc_tag) {
+            g_source_remove(vs->ioc_tag);
+        }
         vs->ioc_tag = qio_channel_add_watch(
             QIO_CHANNEL(vs->ioc), G_IO_IN, vncws_handshake_io, vs, NULL);
     }
@@ -97,6 +100,9 @@ static void vncws_handshake_done(QIOTask *task,
     } else {
         VNC_DEBUG("Websock handshake complete, starting VNC protocol\n");
         vnc_start_protocol(vs);
+        if (vs->ioc_tag) {
+            g_source_remove(vs->ioc_tag);
+        }
         vs->ioc_tag = qio_channel_add_watch(
             vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
     }
diff --git a/ui/vnc.c b/ui/vnc.c
index eb91559b6b..ec86d6ad97 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1121,6 +1121,7 @@ static void vnc_disconnect_start(VncState *vs)
     vnc_set_share_mode(vs, VNC_SHARE_MODE_DISCONNECTED);
     if (vs->ioc_tag) {
         g_source_remove(vs->ioc_tag);
+        vs->ioc_tag = 0;
     }
     qio_channel_close(vs->ioc, NULL);
     vs->disconnecting = TRUE;
@@ -2931,6 +2932,9 @@ static void vnc_connect(VncDisplay *vd, QIOChannelSocket *sioc,
     VNC_DEBUG("New client on socket %p\n", vs->sioc);
     update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
     qio_channel_set_blocking(vs->ioc, false, NULL);
+    if (vs->ioc_tag) {
+        g_source_remove(vs->ioc_tag);
+    }
     if (websocket) {
         vs->websocket = 1;
         if (vd->tlscreds) {
-- 
2.13.3


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

[Qemu-devel] [PATCH] io: Improve websocket support by becoming more RFC compliant.

[Brandon Carpenter]

Remembering the opcode is sufficient for handling fragmented frames from
the client, which may be introduced by an intermediary server/proxy.
Respond to pings and ignore pongs rather than close the connection as
many browsers use ping/pong to test an idle connection. Close
connections according to the RFC, including providing reason code and
message to aid debugging of unexpected disconnects. Empty payloads
should not cause a disconnect.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 include/io/channel-websock.h |   1 +
 io/channel-websock.c         | 243 ++++++++++++++++++++++++++++---------------
 2 files changed, 162 insertions(+), 82 deletions(-)

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index 3c9ff84727..7c896557c5 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -65,6 +65,7 @@ struct QIOChannelWebsock {
     guint io_tag;
     Error *io_err;
     gboolean io_eof;
+    uint8_t opcode;
 };
 
 /**
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 5a3badbec2..45ac2605bb 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -86,8 +86,7 @@
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE 0x0f
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK 0x80
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN 0x7f
-#define QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_FIN 7
-#define QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_HAS_MASK 7
+#define QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK 0x8
 
 typedef struct QIOChannelWebsockHeader QIOChannelWebsockHeader;
 
@@ -123,6 +122,15 @@ enum {
     QIO_CHANNEL_WEBSOCK_OPCODE_PONG = 0xA
 };
 
+enum {
+    QIO_CHANNEL_WEBSOCK_STATUS_NORMAL = 1000,
+    QIO_CHANNEL_WEBSOCK_STATUS_PROTOCOL_ERR = 1002,
+    QIO_CHANNEL_WEBSOCK_STATUS_INVALID_DATA = 1003,
+    QIO_CHANNEL_WEBSOCK_STATUS_POLICY = 1008,
+    QIO_CHANNEL_WEBSOCK_STATUS_TOO_LARGE = 1009,
+    QIO_CHANNEL_WEBSOCK_STATUS_SERVER_ERR = 1011,
+};
+
 static size_t
 qio_channel_websock_extract_headers(char *buffer,
                                     QIOChannelWebsockHTTPHeader *hdrs,
@@ -480,7 +488,8 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
 }
 
 
-static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
+static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
+                                              uint8_t opcode, Buffer *buffer)
 {
     size_t header_size;
     union {
@@ -488,39 +497,63 @@ static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
         QIOChannelWebsockHeader ws;
     } header;
 
-    if (!ioc->rawoutput.offset) {
-        return;
-    }
-
-    header.ws.b0 = (1 << QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_FIN) |
-        (QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &
-         QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
-    if (ioc->rawoutput.offset <
+    header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
+        (opcode & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
+    if (buffer->offset <
         QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
-        header.ws.b1 = (uint8_t)ioc->rawoutput.offset;
+        header.ws.b1 = (uint8_t)buffer->offset;
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
-    } else if (ioc->rawoutput.offset <
+    } else if (buffer->offset <
                QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT;
-        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)ioc->rawoutput.offset);
+        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)buffer->offset);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT;
     } else {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_64_BIT;
-        header.ws.u.s64.l64 = cpu_to_be64(ioc->rawoutput.offset);
+        header.ws.u.s64.l64 = cpu_to_be64(buffer->offset);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT;
     }
     header_size -= QIO_CHANNEL_WEBSOCK_HEADER_LEN_MASK;
 
-    buffer_reserve(&ioc->encoutput, header_size + ioc->rawoutput.offset);
+    buffer_reserve(&ioc->encoutput, header_size + buffer->offset);
     buffer_append(&ioc->encoutput, header.buf, header_size);
-    buffer_append(&ioc->encoutput, ioc->rawoutput.buffer,
-                  ioc->rawoutput.offset);
+    buffer_append(&ioc->encoutput, buffer->buffer, buffer->offset);
+}
+
+
+static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
+{
+    if (!ioc->rawoutput.offset) {
+        return;
+    }
+    qio_channel_websock_encode_buffer(ioc,
+            QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME, &ioc->rawoutput);
     buffer_reset(&ioc->rawoutput);
 }
 
 
-static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
-                                                 Error **errp)
+static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *, Error **);
+
+
+static void qio_channel_websock_write_close(QIOChannelWebsock *ioc,
+                                              uint16_t code, const char *reason)
+{
+    buffer_reserve(&ioc->rawoutput, 2 + (reason ? strlen(reason) : 0));
+    ioc->rawoutput.offset += 2;
+    *(uint16_t *)(ioc->rawoutput.buffer + ioc->rawoutput.offset) = cpu_to_be16(code);
+    if (reason) {
+        buffer_append(&ioc->rawoutput, reason, strlen(reason));
+    }
+    qio_channel_websock_encode_buffer(ioc,
+            QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE, &ioc->rawoutput);
+    buffer_reset(&ioc->rawoutput);
+    qio_channel_websock_write_wire(ioc, NULL);
+    qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+
+
+static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
+                                             Error **errp)
 {
     unsigned char opcode, fin, has_mask;
     size_t header_size;
@@ -529,9 +562,10 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
         (QIOChannelWebsockHeader *)ioc->encinput.buffer;
 
     if (ioc->payload_remain) {
-        error_setg(errp,
-                   "Decoding header but %zu bytes of payload remain",
+        error_setg(errp, "Decoding header but %zu bytes of payload remain",
                    ioc->payload_remain);
+        qio_channel_websock_write_close(ioc,
+                QIO_CHANNEL_WEBSOCK_STATUS_SERVER_ERR, "internal server error");
         return -1;
     }
     if (ioc->encinput.offset < QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT) {
@@ -539,33 +573,49 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
         return QIO_CHANNEL_ERR_BLOCK;
     }
 
-    fin = (header->b0 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN) >>
-        QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_FIN;
+    fin = header->b0 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN;
     opcode = header->b0 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE;
-    has_mask = (header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK) >>
-        QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_HAS_MASK;
+    has_mask = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK;
     payload_len = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN;
 
-    if (opcode == QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE) {
-        /* disconnect */
-        return 0;
+    /* Save or restore opcode. */
+    if (opcode) {
+        ioc->opcode = opcode;
+    } else {
+        opcode = ioc->opcode;
     }
 
     /* Websocket frame sanity check:
-     * * Websocket fragmentation is not supported.
-     * * All  websockets frames sent by a client have to be masked.
-     * * Only binary encoding is supported.
+     * * Only binary and ping/pong encoding is supported.
+     * * Fragmentation is only allowed for binary frames.
+     * * All frames sent by a client MUST be masked.
      */
     if (!fin) {
-        error_setg(errp, "websocket fragmentation is not supported");
-        return -1;
+        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
+            error_setg(errp, "only binary websocket frames may be fragmented");
+            qio_channel_websock_write_close(ioc,
+                    QIO_CHANNEL_WEBSOCK_STATUS_POLICY ,
+                    "only binary frames may be fragmented");
+            return -1;
+        }
+    } else {
+        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &&
+                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE &&
+                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PING &&
+                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PONG) {
+            error_setg(errp, "unsupported opcode: %#04x; only binary, close, "
+                    "ping, and pong websocket frames are supported", opcode);
+            qio_channel_websock_write_close(ioc,
+                    QIO_CHANNEL_WEBSOCK_STATUS_INVALID_DATA ,
+                    "only binary, close, ping, and pong frames are supported");
+            return -1;
+        }
     }
     if (!has_mask) {
-        error_setg(errp, "websocket frames must be masked");
-        return -1;
-    }
-    if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
-        error_setg(errp, "only binary websocket frames are supported");
+        error_setg(errp, "client websocket frames must be masked");
+        qio_channel_websock_write_close(ioc,
+                QIO_CHANNEL_WEBSOCK_STATUS_PROTOCOL_ERR,
+                "client frames must be masked");
         return -1;
     }
 
@@ -573,6 +623,12 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
         ioc->payload_remain = payload_len;
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
         ioc->mask = header->u.m;
+    } else if (opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
+        error_setg(errp, "websocket control frame is too large");
+        qio_channel_websock_write_close(ioc,
+                QIO_CHANNEL_WEBSOCK_STATUS_PROTOCOL_ERR,
+                "control frame is too large");
+        return -1;
     } else if (payload_len == QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT &&
                ioc->encinput.offset >= QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT) {
         ioc->payload_remain = be16_to_cpu(header->u.s16.l16);
@@ -589,53 +645,81 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
     }
 
     buffer_advance(&ioc->encinput, header_size);
-    return 1;
+    return 0;
 }
 
 
-static ssize_t qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
-                                                  Error **errp)
+static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
+                                              Error **errp)
 {
     size_t i;
-    size_t payload_len;
+    size_t payload_len = 0;
     uint32_t *payload32;
 
-    if (!ioc->payload_remain) {
-        error_setg(errp,
-                   "Decoding payload but no bytes of payload remain");
-        return -1;
-    }
+    if (ioc->payload_remain) {
+        /* If we aren't at the end of the payload, then drop
+         * off the last bytes, so we're always multiple of 4
+         * for purpose of unmasking, except at end of payload
+         */
+        if (ioc->encinput.offset < ioc->payload_remain) {
+            /* Wait for the entire payload before processing control frames
+             * because the payload will most likely be echoed back. */
+            if (ioc->opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
+                return QIO_CHANNEL_ERR_BLOCK;
+            }
+            payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
+        } else {
+            payload_len = ioc->payload_remain;
+        }
+        if (payload_len == 0) {
+            return QIO_CHANNEL_ERR_BLOCK;
+        }
 
-    /* If we aren't at the end of the payload, then drop
-     * off the last bytes, so we're always multiple of 4
-     * for purpose of unmasking, except at end of payload
-     */
-    if (ioc->encinput.offset < ioc->payload_remain) {
-        payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
-    } else {
-        payload_len = ioc->payload_remain;
-    }
-    if (payload_len == 0) {
-        return QIO_CHANNEL_ERR_BLOCK;
+        ioc->payload_remain -= payload_len;
+
+        /* unmask frame */
+        /* process 1 frame (32 bit op) */
+        payload32 = (uint32_t *)ioc->encinput.buffer;
+        for (i = 0; i < payload_len / 4; i++) {
+            payload32[i] ^= ioc->mask.u;
+        }
+        /* process the remaining bytes (if any) */
+        for (i *= 4; i < payload_len; i++) {
+            ioc->encinput.buffer[i] ^= ioc->mask.c[i % 4];
+        }
     }
 
-    ioc->payload_remain -= payload_len;
+    if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
+        if (payload_len) {
+            /* binary frames are passed on */
+            buffer_reserve(&ioc->rawinput, payload_len);
+            buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
+        }
+    } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE) {
+        /* close frames are echoed back */
+        error_setg(errp, "websocket closed by peer");
+        if (payload_len) {
+            /* echo client status */
+            qio_channel_websock_encode_buffer(ioc,
+                    QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE, &ioc->encinput);
+            qio_channel_websock_write_wire(ioc, NULL);
+            qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+        } else {
+            /* send our own status */
+            qio_channel_websock_write_close(ioc,
+                    QIO_CHANNEL_WEBSOCK_STATUS_NORMAL, "peer requested close");
+        }
+        return -1;
+    } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
+        /* ping frames produce an immediate pong reply */
+        qio_channel_websock_encode_buffer(ioc,
+                QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
+    }   /* pong frames are ignored */
 
-    /* unmask frame */
-    /* process 1 frame (32 bit op) */
-    payload32 = (uint32_t *)ioc->encinput.buffer;
-    for (i = 0; i < payload_len / 4; i++) {
-        payload32[i] ^= ioc->mask.u;
+    if (payload_len) {
+        buffer_advance(&ioc->encinput, payload_len);
     }
-    /* process the remaining bytes (if any) */
-    for (i *= 4; i < payload_len; i++) {
-        ioc->encinput.buffer[i] ^= ioc->mask.c[i % 4];
-    }
-
-    buffer_reserve(&ioc->rawinput, payload_len);
-    buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
-    buffer_advance(&ioc->encinput, payload_len);
-    return payload_len;
+    return 0;
 }
 
 
@@ -715,8 +799,7 @@ static ssize_t qio_channel_websock_read_wire(QIOChannelWebsock *ioc,
         if (ret < 0) {
             return ret;
         }
-        if (ret == 0 &&
-            ioc->encinput.offset == 0) {
+        if (ret == 0 && ioc->encinput.offset == 0) {
             return 0;
         }
         ioc->encinput.offset += ret;
@@ -725,17 +808,13 @@ static ssize_t qio_channel_websock_read_wire(QIOChannelWebsock *ioc,
     while (ioc->encinput.offset != 0) {
         if (ioc->payload_remain == 0) {
             ret = qio_channel_websock_decode_header(ioc, errp);
-            if (ret < 0) {
+            if (ret) {
                 return ret;
             }
-            if (ret == 0) {
-                ioc->io_eof = TRUE;
-                break;
-            }
         }
 
         ret = qio_channel_websock_decode_payload(ioc, errp);
-        if (ret < 0) {
+        if (ret) {
             return ret;
         }
     }
-- 
2.13.3


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH] io: Improve websocket support by becoming more RFC compliant.

[Daniel P. Berrange]

On Mon, Jul 24, 2017 at 11:42:17AM -0700, Brandon Carpenter wrote:
> Remembering the opcode is sufficient for handling fragmented frames from
> the client, which may be introduced by an intermediary server/proxy.
> Respond to pings and ignore pongs rather than close the connection as
> many browsers use ping/pong to test an idle connection. Close
> connections according to the RFC, including providing reason code and
> message to aid debugging of unexpected disconnects. Empty payloads
> should not cause a disconnect.

Couuld you split this up into multiple patches, each one only fixing
a single problem at a time. It is rather hard to review this for
correctness when everything is changed at once.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] io: Improve websocket support by becoming more RFC compliant.

[Brandon Carpenter]

Sure thing. I implemented it in multiple commits, but there was enough 
overlap in those commits that I thought it would require extra 
unnecessary work to review. But I also found an issue that needs fixed, 
so I need to resubmit anyway.

--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060

On Tue, Jul 25, 2017 at 1:38 AM, Daniel P. Berrange 
<berrange@redhat.com> wrote:
> On Mon, Jul 24, 2017 at 11:42:17AM -0700, Brandon Carpenter wrote:
>>  Remembering the opcode is sufficient for handling fragmented frames 
>> from
>>  the client, which may be introduced by an intermediary server/proxy.
>>  Respond to pings and ignore pongs rather than close the connection 
>> as
>>  many browsers use ping/pong to test an idle connection. Close
>>  connections according to the RFC, including providing reason code 
>> and
>>  message to aid debugging of unexpected disconnects. Empty payloads
>>  should not cause a disconnect.
> 
> Couuld you split this up into multiple patches, each one only fixing
> a single problem at a time. It is rather hard to review this for
> correctness when everything is changed at once.
> 
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    
> https://www.instagram.com/dberrange :|


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

[Qemu-devel] [PATCH v2 0/6] Update websocket code to more fully support the RFC

[Brandon Carpenter]

We've been experiencing issues where the qemu websocket server closes
connections from noVNC clients for no apparent reason. Debugging shows
that certain web browsers are injecting ping and pong frames when the
connection becomes idle. Some browsers send those frames without a
payload, which also is causing closure. This patch series addresses
these issues by making the websocket server more conformant to RFC
6455 - The WebSocket Protocol.

Remembering the opcode is sufficient for handling fragmented frames from
the client, which may be introduced by an intermediary server/proxy.
Respond to pings and ignore pongs rather than close the connection as
many browsers use ping/pong to test an idle connection. Close
connections according to the RFC, including providing a reason code and
message to aid debugging of unexpected disconnects. Empty payloads
should not cause a disconnect.

While updating the websocket code, several other bugs were discovered
for which patches are also included early in the set.

Brandon Carpenter (6):
  io: Always remove an old channel watch before adding a new one
  io: Small updates in preparation for websocket changes
  io: Add support for fragmented websocket binary frames
  io: Allow empty websocket payload
  io: Ignore websocket PING and PONG frames
  io: Reply to ping frames

 include/io/channel-websock.h |   1 +
 io/channel-websock.c         | 207 ++++++++++++++++++++++---------------------
 ui/vnc-auth-vencrypt.c       |   3 +
 ui/vnc-ws.c                  |   6 ++
 ui/vnc.c                     |   4 +
 5 files changed, 122 insertions(+), 99 deletions(-)

-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 0/6] Update websocket code to more fully support the RFC

[Eric Blake]

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

On 09/08/2017 12:37 PM, Brandon Carpenter wrote:
> We've been experiencing issues where the qemu websocket server closes
> connections from noVNC clients for no apparent reason. Debugging shows
> that certain web browsers are injecting ping and pong frames when the
> connection becomes idle. Some browsers send those frames without a
> payload, which also is causing closure. This patch series addresses
> these issues by making the websocket server more conformant to RFC
> 6455 - The WebSocket Protocol.

meta-comment: sending v2 in-reply-to v1 makes it harder for automated
tooling to recognize that you are sending new patches.  We prefer that
v2 be a top-level thread.  More patch submission hints at:
https://wiki.qemu.org/index.php/Contribute/SubmitAPatch

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 0/6] Update websocket code to more fully support the RFC

[Brandon Carpenter]

My apologies. I read that document before submitting the patch, but 
there was a lot to take in. Would you like me to resend the patch 
without the references?
--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060

On Fri, Sep 8, 2017 at 11:01 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/08/2017 12:37 PM, Brandon Carpenter wrote:
>>  We've been experiencing issues where the qemu websocket server 
>> closes
>>  connections from noVNC clients for no apparent reason. Debugging 
>> shows
>>  that certain web browsers are injecting ping and pong frames when 
>> the
>>  connection becomes idle. Some browsers send those frames without a
>>  payload, which also is causing closure. This patch series addresses
>>  these issues by making the websocket server more conformant to RFC
>>  6455 - The WebSocket Protocol.
> 
> meta-comment: sending v2 in-reply-to v1 makes it harder for automated
> tooling to recognize that you are sending new patches.  We prefer that
> v2 be a top-level thread.  More patch submission hints at:
> https://wiki.qemu.org/index.php/Contribute/SubmitAPatch
> 
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 

-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 0/6] Update websocket code to more fully support the RFC

[Eric Blake]

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

On 09/08/2017 01:11 PM, Brandon Carpenter wrote:
> My apologies. I read that document before submitting the patch, but
> there was a lot to take in. Would you like me to resend the patch
> without the references?

Not a problem - we all had to submit our first patch at some point in
time in the past :)

Re-sending at this point shouldn't be necessary unless a maintainer
specifically asks for it; it's more information for next time (if you
have to send a v3, for whatever reason).


> 
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
> is for the sole use of the intended recipient(s) and may contain
> proprietary, confidential or privileged information or otherwise be
> protected by law. Any unauthorized review, use, disclosure or
> distribution is prohibited. If you are not the intended recipient,
> please notify the sender and destroy all copies and the original message.

By the way, your employer's disclaimer is unenforceable on a
publicly-archived mailing list; still, it may be worth replying from a
person account to avoid the risk of someone refusing to reply to your
mail on principle.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH v2 1/6] io: Always remove an old channel watch before adding a new one

[Brandon Carpenter]

Also set saved handle to zero when removing without adding a new watch.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc-auth-vencrypt.c | 3 +++
 ui/vnc-ws.c            | 6 ++++++
 ui/vnc.c               | 4 ++++
 3 files changed, 13 insertions(+)

diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
index ffaab57550..c3eece4fa7 100644
--- a/ui/vnc-auth-vencrypt.c
+++ b/ui/vnc-auth-vencrypt.c
@@ -77,6 +77,9 @@ static void vnc_tls_handshake_done(QIOTask *task,
         vnc_client_error(vs);
         error_free(err);
     } else {
+        if (vs->ioc_tag) {
+            g_source_remove(vs->ioc_tag);
+        }
         vs->ioc_tag = qio_channel_add_watch(
             vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
         start_auth_vencrypt_subauth(vs);
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index f530cd5474..eaf309553c 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -36,6 +36,9 @@ static void vncws_tls_handshake_done(QIOTask *task,
         error_free(err);
     } else {
         VNC_DEBUG("TLS handshake complete, starting websocket handshake\n");
+        if (vs->ioc_tag) {
+            g_source_remove(vs->ioc_tag);
+        }
         vs->ioc_tag = qio_channel_add_watch(
             QIO_CHANNEL(vs->ioc), G_IO_IN, vncws_handshake_io, vs, NULL);
     }
@@ -97,6 +100,9 @@ static void vncws_handshake_done(QIOTask *task,
     } else {
         VNC_DEBUG("Websock handshake complete, starting VNC protocol\n");
         vnc_start_protocol(vs);
+        if (vs->ioc_tag) {
+            g_source_remove(vs->ioc_tag);
+        }
         vs->ioc_tag = qio_channel_add_watch(
             vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
     }
diff --git a/ui/vnc.c b/ui/vnc.c
index 0b5dbc62e4..62f7a3f30a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1121,6 +1121,7 @@ static void vnc_disconnect_start(VncState *vs)
     vnc_set_share_mode(vs, VNC_SHARE_MODE_DISCONNECTED);
     if (vs->ioc_tag) {
         g_source_remove(vs->ioc_tag);
+        vs->ioc_tag = 0;
     }
     qio_channel_close(vs->ioc, NULL);
     vs->disconnecting = TRUE;
@@ -2931,6 +2932,9 @@ static void vnc_connect(VncDisplay *vd, QIOChannelSocket *sioc,
     VNC_DEBUG("New client on socket %p\n", vs->sioc);
     update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
     qio_channel_set_blocking(vs->ioc, false, NULL);
+    if (vs->ioc_tag) {
+        g_source_remove(vs->ioc_tag);
+    }
     if (websocket) {
         vs->websocket = 1;
         if (vd->tlscreds) {
-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

[Qemu-devel] [PATCH v2 2/6] io: Small updates in preparation for websocket changes

[Brandon Carpenter]

Gets rid of unnecessary bit shifting and performs proper EOF checking to
avoid a large number of repeated calls to recvmsg() when a client
abruptly terminates a connection (bug fix).

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 io/channel-websock.c | 62 +++++++++++++++-------------------------------------
 1 file changed, 18 insertions(+), 44 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 5a3badbec2..185bd31be5 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -86,8 +86,6 @@
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE 0x0f
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK 0x80
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN 0x7f
-#define QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_FIN 7
-#define QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_HAS_MASK 7
 
 typedef struct QIOChannelWebsockHeader QIOChannelWebsockHeader;
 
@@ -492,7 +490,7 @@ static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
         return;
     }
 
-    header.ws.b0 = (1 << QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_FIN) |
+    header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
         (QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &
          QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
     if (ioc->rawoutput.offset <
@@ -519,8 +517,8 @@ static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
 }
 
 
-static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
-                                                 Error **errp)
+static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
+                                             Error **errp)
 {
     unsigned char opcode, fin, has_mask;
     size_t header_size;
@@ -539,11 +537,9 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
         return QIO_CHANNEL_ERR_BLOCK;
     }
 
-    fin = (header->b0 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN) >>
-        QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_FIN;
+    fin = header->b0 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN;
     opcode = header->b0 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE;
-    has_mask = (header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK) >>
-        QIO_CHANNEL_WEBSOCK_HEADER_SHIFT_HAS_MASK;
+    has_mask = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK;
     payload_len = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN;
 
     if (opcode == QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE) {
@@ -561,7 +557,7 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
         return -1;
     }
     if (!has_mask) {
-        error_setg(errp, "websocket frames must be masked");
+        error_setg(errp, "client websocket frames must be masked");
         return -1;
     }
     if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
@@ -593,8 +589,8 @@ static ssize_t qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
 }
 
 
-static ssize_t qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
-                                                  Error **errp)
+static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
+                                              Error **errp)
 {
     size_t i;
     size_t payload_len;
@@ -635,7 +631,7 @@ static ssize_t qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
     buffer_reserve(&ioc->rawinput, payload_len);
     buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
     buffer_advance(&ioc->encinput, payload_len);
-    return payload_len;
+    return 0;
 }
 
 
@@ -715,8 +711,8 @@ static ssize_t qio_channel_websock_read_wire(QIOChannelWebsock *ioc,
         if (ret < 0) {
             return ret;
         }
-        if (ret == 0 &&
-            ioc->encinput.offset == 0) {
+        if (ret == 0 && ioc->encinput.offset == 0) {
+            ioc->io_eof = TRUE;
             return 0;
         }
         ioc->encinput.offset += ret;
@@ -728,10 +724,6 @@ static ssize_t qio_channel_websock_read_wire(QIOChannelWebsock *ioc,
             if (ret < 0) {
                 return ret;
             }
-            if (ret == 0) {
-                ioc->io_eof = TRUE;
-                break;
-            }
         }
 
         ret = qio_channel_websock_decode_payload(ioc, errp);
@@ -996,14 +988,12 @@ struct QIOChannelWebsockSource {
 };
 
 static gboolean
-qio_channel_websock_source_prepare(GSource *source,
-                                   gint *timeout)
+qio_channel_websock_source_check(GSource *source)
 {
     QIOChannelWebsockSource *wsource = (QIOChannelWebsockSource *)source;
     GIOCondition cond = 0;
-    *timeout = -1;
 
-    if (wsource->wioc->rawinput.offset) {
+    if (wsource->wioc->rawinput.offset || wsource->wioc->io_eof) {
         cond |= G_IO_IN;
     }
     if (wsource->wioc->rawoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
@@ -1014,19 +1004,11 @@ qio_channel_websock_source_prepare(GSource *source,
 }
 
 static gboolean
-qio_channel_websock_source_check(GSource *source)
+qio_channel_websock_source_prepare(GSource *source,
+                                   gint *timeout)
 {
-    QIOChannelWebsockSource *wsource = (QIOChannelWebsockSource *)source;
-    GIOCondition cond = 0;
-
-    if (wsource->wioc->rawinput.offset) {
-        cond |= G_IO_IN;
-    }
-    if (wsource->wioc->rawoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
-        cond |= G_IO_OUT;
-    }
-
-    return cond & wsource->condition;
+    *timeout = -1;
+    return qio_channel_websock_source_check(source);
 }
 
 static gboolean
@@ -1036,17 +1018,9 @@ qio_channel_websock_source_dispatch(GSource *source,
 {
     QIOChannelFunc func = (QIOChannelFunc)callback;
     QIOChannelWebsockSource *wsource = (QIOChannelWebsockSource *)source;
-    GIOCondition cond = 0;
-
-    if (wsource->wioc->rawinput.offset) {
-        cond |= G_IO_IN;
-    }
-    if (wsource->wioc->rawoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
-        cond |= G_IO_OUT;
-    }
 
     return (*func)(QIO_CHANNEL(wsource->wioc),
-                   (cond & wsource->condition),
+                   qio_channel_websock_source_check(source),
                    user_data);
 }
 
-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

[Qemu-devel] [PATCH v2 3/6] io: Add support for fragmented websocket binary frames

[Brandon Carpenter]

Allows fragmented binary frames by saving the previous opcode. Handles
the case where an intermediary (i.e., web proxy) fragments frames
originally sent unfragmented by the client.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 include/io/channel-websock.h |  1 +
 io/channel-websock.c         | 26 ++++++++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index 3c9ff84727..7c896557c5 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -65,6 +65,7 @@ struct QIOChannelWebsock {
     guint io_tag;
     Error *io_err;
     gboolean io_eof;
+    uint8_t opcode;
 };
 
 /**
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 185bd31be5..ced24135ec 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -542,28 +542,38 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
     has_mask = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK;
     payload_len = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN;
 
+    /* Save or restore opcode. */
+    if (opcode) {
+        ioc->opcode = opcode;
+    } else {
+        opcode = ioc->opcode;
+    }
+
     if (opcode == QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE) {
         /* disconnect */
         return 0;
     }
 
     /* Websocket frame sanity check:
-     * * Websocket fragmentation is not supported.
-     * * All  websockets frames sent by a client have to be masked.
+     * * Fragmentation is only supported for binary frames.
+     * * All frames sent by a client MUST be masked.
      * * Only binary encoding is supported.
      */
     if (!fin) {
-        error_setg(errp, "websocket fragmentation is not supported");
-        return -1;
+        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
+            error_setg(errp, "only binary websocket frames may be fragmented");
+            return -1;
+        }
+    } else {
+        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
+            error_setg(errp, "only binary websocket frames are supported");
+            return -1;
+        }
     }
     if (!has_mask) {
         error_setg(errp, "client websocket frames must be masked");
         return -1;
     }
-    if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
-        error_setg(errp, "only binary websocket frames are supported");
-        return -1;
-    }
 
     if (payload_len < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT) {
         ioc->payload_remain = payload_len;
-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

[Qemu-devel] [PATCH v2 4/6] io: Allow empty websocket payload

[Brandon Carpenter]

Some browsers send pings/pongs with no payload, so allow empty payloads
instead of closing the connection.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 io/channel-websock.c | 62 +++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index ced24135ec..3183aeff77 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -603,44 +603,42 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
                                               Error **errp)
 {
     size_t i;
-    size_t payload_len;
+    size_t payload_len = 0;
     uint32_t *payload32;
 
-    if (!ioc->payload_remain) {
-        error_setg(errp,
-                   "Decoding payload but no bytes of payload remain");
-        return -1;
-    }
-
-    /* If we aren't at the end of the payload, then drop
-     * off the last bytes, so we're always multiple of 4
-     * for purpose of unmasking, except at end of payload
-     */
-    if (ioc->encinput.offset < ioc->payload_remain) {
-        payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
-    } else {
-        payload_len = ioc->payload_remain;
-    }
-    if (payload_len == 0) {
-        return QIO_CHANNEL_ERR_BLOCK;
-    }
+    if (ioc->payload_remain) {
+        /* If we aren't at the end of the payload, then drop
+         * off the last bytes, so we're always multiple of 4
+         * for purpose of unmasking, except at end of payload
+         */
+        if (ioc->encinput.offset < ioc->payload_remain) {
+            payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
+        } else {
+            payload_len = ioc->payload_remain;
+        }
+        if (payload_len == 0) {
+            return QIO_CHANNEL_ERR_BLOCK;
+        }
 
-    ioc->payload_remain -= payload_len;
+        ioc->payload_remain -= payload_len;
 
-    /* unmask frame */
-    /* process 1 frame (32 bit op) */
-    payload32 = (uint32_t *)ioc->encinput.buffer;
-    for (i = 0; i < payload_len / 4; i++) {
-        payload32[i] ^= ioc->mask.u;
-    }
-    /* process the remaining bytes (if any) */
-    for (i *= 4; i < payload_len; i++) {
-        ioc->encinput.buffer[i] ^= ioc->mask.c[i % 4];
+        /* unmask frame */
+        /* process 1 frame (32 bit op) */
+        payload32 = (uint32_t *)ioc->encinput.buffer;
+        for (i = 0; i < payload_len / 4; i++) {
+            payload32[i] ^= ioc->mask.u;
+        }
+        /* process the remaining bytes (if any) */
+        for (i *= 4; i < payload_len; i++) {
+            ioc->encinput.buffer[i] ^= ioc->mask.c[i % 4];
+        }
     }
 
-    buffer_reserve(&ioc->rawinput, payload_len);
-    buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
-    buffer_advance(&ioc->encinput, payload_len);
+    if (payload_len) {
+        buffer_reserve(&ioc->rawinput, payload_len);
+        buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
+        buffer_advance(&ioc->encinput, payload_len);
+    }
     return 0;
 }
 
-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

[Qemu-devel] [PATCH v2 5/6] io: Ignore websocket PING and PONG frames

[Brandon Carpenter]

Keep pings and gratuitous pongs generated by web browsers from killing
websocket connections.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 io/channel-websock.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 3183aeff77..50387050d5 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -86,6 +86,7 @@
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE 0x0f
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK 0x80
 #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN 0x7f
+#define QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK 0x8
 
 typedef struct QIOChannelWebsockHeader QIOChannelWebsockHeader;
 
@@ -565,8 +566,11 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
             return -1;
         }
     } else {
-        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
-            error_setg(errp, "only binary websocket frames are supported");
+        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &&
+                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PING &&
+                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PONG) {
+            error_setg(errp, "unsupported opcode: %#04x; only binary, ping, "
+                             "and pong websocket frames are supported", opcode);
             return -1;
         }
     }
@@ -579,6 +583,9 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
         ioc->payload_remain = payload_len;
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
         ioc->mask = header->u.m;
+    } else if (opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
+        error_setg(errp, "websocket control frame is too large");
+        return -1;
     } else if (payload_len == QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT &&
                ioc->encinput.offset >= QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT) {
         ioc->payload_remain = be16_to_cpu(header->u.s16.l16);
@@ -634,9 +641,15 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         }
     }
 
+    /* Drop the payload of ping/pong packets */
+    if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
+        if (payload_len) {
+            buffer_reserve(&ioc->rawinput, payload_len);
+            buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
+        }
+    }
+
     if (payload_len) {
-        buffer_reserve(&ioc->rawinput, payload_len);
-        buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
         buffer_advance(&ioc->encinput, payload_len);
     }
     return 0;
-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 5/6] io: Ignore websocket PING and PONG frames

[Daniel P. Berrange]

On Fri, Sep 08, 2017 at 10:38:00AM -0700, Brandon Carpenter wrote:
> Keep pings and gratuitous pongs generated by web browsers from killing
> websocket connections.
> 
> Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> ---
>  io/channel-websock.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 3183aeff77..50387050d5 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -86,6 +86,7 @@
>  #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE 0x0f
>  #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK 0x80
>  #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN 0x7f
> +#define QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK 0x8
>  
>  typedef struct QIOChannelWebsockHeader QIOChannelWebsockHeader;
>  
> @@ -565,8 +566,11 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
>              return -1;
>          }
>      } else {
> -        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
> -            error_setg(errp, "only binary websocket frames are supported");
> +        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &&
> +                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PING &&
> +                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PONG) {

Why would we need to ignore PONG ?  A client should only send a PONG in
response to a PING that we send, and we never send PINGs.  So if we
received a PONG that would be a serious error by the client, which should
cause us to close the connection IMHO>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 5/6] io: Ignore websocket PING and PONG frames

[Daniel P. Berrange]

On Mon, Sep 11, 2017 at 09:38:46AM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 08, 2017 at 10:38:00AM -0700, Brandon Carpenter wrote:
> > Keep pings and gratuitous pongs generated by web browsers from killing
> > websocket connections.
> > 
> > Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> > ---
> >  io/channel-websock.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/io/channel-websock.c b/io/channel-websock.c
> > index 3183aeff77..50387050d5 100644
> > --- a/io/channel-websock.c
> > +++ b/io/channel-websock.c
> > @@ -86,6 +86,7 @@
> >  #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE 0x0f
> >  #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_HAS_MASK 0x80
> >  #define QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN 0x7f
> > +#define QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK 0x8
> >  
> >  typedef struct QIOChannelWebsockHeader QIOChannelWebsockHeader;
> >  
> > @@ -565,8 +566,11 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
> >              return -1;
> >          }
> >      } else {
> > -        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
> > -            error_setg(errp, "only binary websocket frames are supported");
> > +        if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &&
> > +                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PING &&
> > +                opcode != QIO_CHANNEL_WEBSOCK_OPCODE_PONG) {
> 
> Why would we need to ignore PONG ?  A client should only send a PONG in
> response to a PING that we send, and we never send PINGs.  So if we
> received a PONG that would be a serious error by the client, which should
> cause us to close the connection IMHO>

Never mind, I've just seen that the RFC allows clients to send an
unsolicited PONG


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Brandon Carpenter]

Add an immediate ping reply (pong) to the outgoing stream when a ping
is received. Unsolicited pongs are ignored.

Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
---
 io/channel-websock.c | 50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 50387050d5..175f17ce6b 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -479,7 +479,8 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
 }
 
 
-static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
+static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
+                                              uint8_t opcode, Buffer *buffer)
 {
     size_t header_size;
     union {
@@ -487,33 +488,37 @@ static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
         QIOChannelWebsockHeader ws;
     } header;
 
-    if (!ioc->rawoutput.offset) {
-        return;
-    }
-
     header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
-        (QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &
-         QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
-    if (ioc->rawoutput.offset <
+        (opcode & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
+    if (buffer->offset <
         QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
-        header.ws.b1 = (uint8_t)ioc->rawoutput.offset;
+        header.ws.b1 = (uint8_t)buffer->offset;
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
-    } else if (ioc->rawoutput.offset <
+    } else if (buffer->offset <
                QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT;
-        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)ioc->rawoutput.offset);
+        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)buffer->offset);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT;
     } else {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_64_BIT;
-        header.ws.u.s64.l64 = cpu_to_be64(ioc->rawoutput.offset);
+        header.ws.u.s64.l64 = cpu_to_be64(buffer->offset);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT;
     }
     header_size -= QIO_CHANNEL_WEBSOCK_HEADER_LEN_MASK;
 
-    buffer_reserve(&ioc->encoutput, header_size + ioc->rawoutput.offset);
+    buffer_reserve(&ioc->encoutput, header_size + buffer->offset);
     buffer_append(&ioc->encoutput, header.buf, header_size);
-    buffer_append(&ioc->encoutput, ioc->rawoutput.buffer,
-                  ioc->rawoutput.offset);
+    buffer_append(&ioc->encoutput, buffer->buffer, buffer->offset);
+}
+
+
+static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
+{
+    if (!ioc->rawoutput.offset) {
+        return;
+    }
+    qio_channel_websock_encode_buffer(ioc,
+            QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME, &ioc->rawoutput);
     buffer_reset(&ioc->rawoutput);
 }
 
@@ -558,7 +563,7 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
     /* Websocket frame sanity check:
      * * Fragmentation is only supported for binary frames.
      * * All frames sent by a client MUST be masked.
-     * * Only binary encoding is supported.
+     * * Only binary and ping/pong encoding is supported.
      */
     if (!fin) {
         if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
@@ -619,6 +624,11 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
          * for purpose of unmasking, except at end of payload
          */
         if (ioc->encinput.offset < ioc->payload_remain) {
+            /* Wait for the entire payload before processing control frames
+             * because the payload will most likely be echoed back. */
+            if (ioc->opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
+                return QIO_CHANNEL_ERR_BLOCK;
+            }
             payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
         } else {
             payload_len = ioc->payload_remain;
@@ -641,13 +651,17 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         }
     }
 
-    /* Drop the payload of ping/pong packets */
     if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
         if (payload_len) {
+            /* binary frames are passed on */
             buffer_reserve(&ioc->rawinput, payload_len);
             buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
         }
-    }
+    } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
+        /* ping frames produce an immediate pong reply */
+        qio_channel_websock_encode_buffer(ioc,
+                QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
+    }   /* pong frames are ignored */
 
     if (payload_len) {
         buffer_advance(&ioc->encinput, payload_len);
-- 
2.14.1


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Daniel P. Berrange]

On Fri, Sep 08, 2017 at 10:38:01AM -0700, Brandon Carpenter wrote:
> Add an immediate ping reply (pong) to the outgoing stream when a ping
> is received. Unsolicited pongs are ignored.
> 
> Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> ---
>  io/channel-websock.c | 50 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 50387050d5..175f17ce6b 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -479,7 +479,8 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
>  }
>  
>  
> -static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
> +static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
> +                                              uint8_t opcode, Buffer *buffer)
>  {
>      size_t header_size;
>      union {
> @@ -487,33 +488,37 @@ static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
>          QIOChannelWebsockHeader ws;
>      } header;
>  
> -    if (!ioc->rawoutput.offset) {
> -        return;
> -    }
> -
>      header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
> -        (QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &
> -         QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
> -    if (ioc->rawoutput.offset <
> +        (opcode & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
> +    if (buffer->offset <
>          QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
> -        header.ws.b1 = (uint8_t)ioc->rawoutput.offset;
> +        header.ws.b1 = (uint8_t)buffer->offset;
>          header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
> -    } else if (ioc->rawoutput.offset <
> +    } else if (buffer->offset <
>                 QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
>          header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT;
> -        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)ioc->rawoutput.offset);
> +        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)buffer->offset);
>          header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT;
>      } else {
>          header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_64_BIT;
> -        header.ws.u.s64.l64 = cpu_to_be64(ioc->rawoutput.offset);
> +        header.ws.u.s64.l64 = cpu_to_be64(buffer->offset);
>          header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT;
>      }
>      header_size -= QIO_CHANNEL_WEBSOCK_HEADER_LEN_MASK;
>  
> -    buffer_reserve(&ioc->encoutput, header_size + ioc->rawoutput.offset);
> +    buffer_reserve(&ioc->encoutput, header_size + buffer->offset);
>      buffer_append(&ioc->encoutput, header.buf, header_size);
> -    buffer_append(&ioc->encoutput, ioc->rawoutput.buffer,
> -                  ioc->rawoutput.offset);
> +    buffer_append(&ioc->encoutput, buffer->buffer, buffer->offset);
> +}
> +
> +
> +static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
> +{
> +    if (!ioc->rawoutput.offset) {
> +        return;
> +    }
> +    qio_channel_websock_encode_buffer(ioc,
> +            QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME, &ioc->rawoutput);
>      buffer_reset(&ioc->rawoutput);
>  }
>  
> @@ -558,7 +563,7 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
>      /* Websocket frame sanity check:
>       * * Fragmentation is only supported for binary frames.
>       * * All frames sent by a client MUST be masked.
> -     * * Only binary encoding is supported.
> +     * * Only binary and ping/pong encoding is supported.
>       */
>      if (!fin) {
>          if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
> @@ -619,6 +624,11 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
>           * for purpose of unmasking, except at end of payload
>           */
>          if (ioc->encinput.offset < ioc->payload_remain) {
> +            /* Wait for the entire payload before processing control frames
> +             * because the payload will most likely be echoed back. */
> +            if (ioc->opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
> +                return QIO_CHANNEL_ERR_BLOCK;
> +            }
>              payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
>          } else {
>              payload_len = ioc->payload_remain;
> @@ -641,13 +651,17 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
>          }
>      }
>  
> -    /* Drop the payload of ping/pong packets */
>      if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
>          if (payload_len) {
> +            /* binary frames are passed on */
>              buffer_reserve(&ioc->rawinput, payload_len);
>              buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
>          }
> -    }
> +    } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> +        /* ping frames produce an immediate pong reply */
> +        qio_channel_websock_encode_buffer(ioc,
> +                QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
> +    }   /* pong frames are ignored */

So qio_channel_websock_opcode_pong() takes the PING payload and adds it
to ioc->encoutput buffer.  When the socket is later seen as writable
the event loop will write this buffer to the wire.

I'm concerned that there is no rate limiting here though, so if a large
number of PINGs are sent, and writing of the reply blocks for some reason,
encoutput will grow without bounds.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Brandon Carpenter]

On Mon, Sep 11, 2017 at 1:50 AM, Daniel P. Berrange 
<berrange@redhat.com> wrote:
> I'm concerned that there is no rate limiting here though, so if a 
> large number of PINGs are sent, and writing of the reply blocks for 
> some reason, encoutput will grow without bounds.

That is a good point. How about something like this to fix it?

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index 7c896557c5..c5a8c3e96c 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -66,6 +66,7 @@ struct QIOChannelWebsock {
     Error *io_err;
     gboolean io_eof;
     uint8_t opcode;
+ uint8_t prev_opcode;
 };

 /**
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 175f17ce6b..a9315c01fb 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -549,6 +549,7 @@ static int 
qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
     payload_len = header->b1 & 
QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN;

     /* Save or restore opcode. */
+ ioc->prev_opcode = ioc->opcode;
     if (opcode) {
         ioc->opcode = opcode;
     } else {
@@ -658,9 +659,14 @@ static int 
qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
             buffer_append(&ioc->rawinput, ioc->encinput.buffer, 
payload_len);
         }
     } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
- /* ping frames produce an immediate pong reply */
- qio_channel_websock_encode_buffer(ioc,
- QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
+ /* Ping frames produce an immediate pong reply, unless one
+ * is already queued, in which case they are coalesced
+ * to avoid unbounded buffer growth.
+ */
+ if (!ioc->encoutput.offset || ioc->prev_opcode != 
QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
+ qio_channel_websock_encode_buffer(ioc,
+ QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
+ }
     } /* pong frames are ignored */

     if (payload_len) {

--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Daniel P. Berrange]

On Mon, Sep 11, 2017 at 10:03:35AM -0700, Brandon Carpenter wrote:
> On Mon, Sep 11, 2017 at 1:50 AM, Daniel P. Berrange <berrange@redhat.com>
> wrote:
> > I'm concerned that there is no rate limiting here though, so if a large
> > number of PINGs are sent, and writing of the reply blocks for some
> > reason, encoutput will grow without bounds.
> 
> That is a good point. How about something like this to fix it?
> 
> diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
> index 7c896557c5..c5a8c3e96c 100644
> --- a/include/io/channel-websock.h
> +++ b/include/io/channel-websock.h
> @@ -66,6 +66,7 @@ struct QIOChannelWebsock {
>     Error *io_err;
>     gboolean io_eof;
>     uint8_t opcode;
> + uint8_t prev_opcode;
> };
> 
> /**
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 175f17ce6b..a9315c01fb 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -549,6 +549,7 @@ static int
> qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
>     payload_len = header->b1 & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_PAYLOAD_LEN;
> 
>     /* Save or restore opcode. */
> + ioc->prev_opcode = ioc->opcode;
>     if (opcode) {
>         ioc->opcode = opcode;
>     } else {
> @@ -658,9 +659,14 @@ static int
> qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
>             buffer_append(&ioc->rawinput, ioc->encinput.buffer,
> payload_len);
>         }
>     } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> - /* ping frames produce an immediate pong reply */
> - qio_channel_websock_encode_buffer(ioc,
> - QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
> + /* Ping frames produce an immediate pong reply, unless one
> + * is already queued, in which case they are coalesced
> + * to avoid unbounded buffer growth.
> + */
> + if (!ioc->encoutput.offset || ioc->prev_opcode !=
> QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> + qio_channel_websock_encode_buffer(ioc,
> + QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
> + }

It feels like this is still dangerous - the client simply has to
interleave each "ping" with a 1 byte binary frame to get around
this limit. We need to make sure we have an absolute cap on the
output buffer size. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Brandon Carpenter]

On Mon, Sep 11, 2017 at 10:10 AM, Daniel P. Berrange 
<berrange@redhat.com> wrote:
> It feels like this is still dangerous - the client simply has to 
> interleave each "ping" with a 1 byte binary frame to get around this 
> limit. We need to make sure we have an absolute cap on the output 
> buffer size.

Okay. I see that now that I look at it more closely. This breed of 
asynchronous I/O is tricky because the conditions for reading/writing 
are all over the place. There's a lot of context to keep in your head.

I have a fix. And I realized that I was missing a patch in the series 
for RFC-compliant closing of websocket connections, which I must have 
lost during a rebase. Should I submit v3 of the patch series or just 
add those patches to this thread?

Thank you,
--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Daniel P. Berrange]

On Mon, Sep 11, 2017 at 12:04:15PM -0700, Brandon Carpenter wrote:
> On Mon, Sep 11, 2017 at 10:10 AM, Daniel P. Berrange <berrange@redhat.com>
> wrote:
> > It feels like this is still dangerous - the client simply has to
> > interleave each "ping" with a 1 byte binary frame to get around this
> > limit. We need to make sure we have an absolute cap on the output buffer
> > size.
> 
> Okay. I see that now that I look at it more closely. This breed of
> asynchronous I/O is tricky because the conditions for reading/writing are
> all over the place. There's a lot of context to keep in your head.
> 
> I have a fix. And I realized that I was missing a patch in the series for
> RFC-compliant closing of websocket connections, which I must have lost
> during a rebase. Should I submit v3 of the patch series or just add those
> patches to this thread?

It is generally preferred practice to submit new top level threads, rather
than sending more patches to a previous thread.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Daniel P. Berrange]

On Fri, Sep 08, 2017 at 10:38:01AM -0700, Brandon Carpenter wrote:
> Add an immediate ping reply (pong) to the outgoing stream when a ping
> is received. Unsolicited pongs are ignored.
> 
> Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> ---
>  io/channel-websock.c | 50 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index 50387050d5..175f17ce6b 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -479,7 +479,8 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
>  }
>  
>  
> -static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
> +static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
> +                                              uint8_t opcode, Buffer *buffer)
>  {
>      size_t header_size;
>      union {
> @@ -487,33 +488,37 @@ static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
>          QIOChannelWebsockHeader ws;
>      } header;
>  
> -    if (!ioc->rawoutput.offset) {
> -        return;
> -    }
> -
>      header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
> -        (QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME &
> -         QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
> -    if (ioc->rawoutput.offset <
> +        (opcode & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
> +    if (buffer->offset <
>          QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
> -        header.ws.b1 = (uint8_t)ioc->rawoutput.offset;
> +        header.ws.b1 = (uint8_t)buffer->offset;
>          header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
> -    } else if (ioc->rawoutput.offset <
> +    } else if (buffer->offset <
>                 QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
>          header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT;
> -        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)ioc->rawoutput.offset);
> +        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)buffer->offset);
>          header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT;
>      } else {
>          header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_64_BIT;
> -        header.ws.u.s64.l64 = cpu_to_be64(ioc->rawoutput.offset);
> +        header.ws.u.s64.l64 = cpu_to_be64(buffer->offset);
>          header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT;
>      }
>      header_size -= QIO_CHANNEL_WEBSOCK_HEADER_LEN_MASK;
>  
> -    buffer_reserve(&ioc->encoutput, header_size + ioc->rawoutput.offset);
> +    buffer_reserve(&ioc->encoutput, header_size + buffer->offset);
>      buffer_append(&ioc->encoutput, header.buf, header_size);
> -    buffer_append(&ioc->encoutput, ioc->rawoutput.buffer,
> -                  ioc->rawoutput.offset);
> +    buffer_append(&ioc->encoutput, buffer->buffer, buffer->offset);
> +}
> +
> +
> +static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
> +{
> +    if (!ioc->rawoutput.offset) {
> +        return;
> +    }
> +    qio_channel_websock_encode_buffer(ioc,
> +            QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME, &ioc->rawoutput);
>      buffer_reset(&ioc->rawoutput);
>  }
>  
> @@ -558,7 +563,7 @@ static int qio_channel_websock_decode_header(QIOChannelWebsock *ioc,
>      /* Websocket frame sanity check:
>       * * Fragmentation is only supported for binary frames.
>       * * All frames sent by a client MUST be masked.
> -     * * Only binary encoding is supported.
> +     * * Only binary and ping/pong encoding is supported.
>       */
>      if (!fin) {
>          if (opcode != QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
> @@ -619,6 +624,11 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
>           * for purpose of unmasking, except at end of payload
>           */
>          if (ioc->encinput.offset < ioc->payload_remain) {
> +            /* Wait for the entire payload before processing control frames
> +             * because the payload will most likely be echoed back. */
> +            if (ioc->opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
> +                return QIO_CHANNEL_ERR_BLOCK;
> +            }
>              payload_len = ioc->encinput.offset - (ioc->encinput.offset % 4);
>          } else {
>              payload_len = ioc->payload_remain;
> @@ -641,13 +651,17 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
>          }
>      }
>  
> -    /* Drop the payload of ping/pong packets */
>      if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME) {
>          if (payload_len) {
> +            /* binary frames are passed on */
>              buffer_reserve(&ioc->rawinput, payload_len);
>              buffer_append(&ioc->rawinput, ioc->encinput.buffer, payload_len);
>          }
> -    }
> +    } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> +        /* ping frames produce an immediate pong reply */
> +        qio_channel_websock_encode_buffer(ioc,
> +                QIO_CHANNEL_WEBSOCK_OPCODE_PONG, &ioc->encinput);
> +    }   /* pong frames are ignored */

BTW, replying to the PING here is going to scramble the protocol if
the PING contains payload data. At the time qio_channel_websock_decode_header
is run, 'encinput' is only guaranteed to contain enough data to decode the
header.  You might be lucky and have some bytes from the payload already
read off the wire and stored in 'encinput', but equally you might have
nothing.  So you can not access 'encinput' here for your PONG reply
payload.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Brandon Carpenter]

On Mon, Sep 11, 2017 at 10:37 AM, Daniel P. Berrange 
<berrange@redhat.com> wrote:
> At the time qio_channel_websock_decode_header is run, 'encinput' is 
> only guaranteed to contain enough data to decode the header.

Because the PING opcode is a control frame, this bit of code earlier in 
the function will ensure the entire frame has been read before the PING 
processing occurs:

>       if (ioc->encinput.offset < ioc->payload_remain) {
>             /* Wait for the entire payload before processing control 
> frames
>              * because the payload will most likely be echoed back. */
>             if (ioc->opcode & 
> QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
>                 return QIO_CHANNEL_ERR_BLOCK;
>             }
>             payload_len = ioc->encinput.offset - 
> (ioc->encinput.offset % 4);

--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Daniel P. Berrange]

On Mon, Sep 11, 2017 at 10:43:21AM -0700, Brandon Carpenter wrote:
> On Mon, Sep 11, 2017 at 10:37 AM, Daniel P. Berrange <berrange@redhat.com>
> wrote:
> > At the time qio_channel_websock_decode_header is run, 'encinput' is only
> > guaranteed to contain enough data to decode the header.
> 
> Because the PING opcode is a control frame, this bit of code earlier in the
> function will ensure the entire frame has been read before the PING
> processing occurs:
> 
> >       if (ioc->encinput.offset < ioc->payload_remain) {
> >             /* Wait for the entire payload before processing control
> > frames
> >              * because the payload will most likely be echoed back. */
> >             if (ioc->opcode & QIO_CHANNEL_WEBSOCK_CONTROL_OPCODE_MASK) {
> >                 return QIO_CHANNEL_ERR_BLOCK;
> >             }
> >             payload_len = ioc->encinput.offset - (ioc->encinput.offset %
> > 4);

The problem is in the qio_channel_websock_read_wire method we refuse
to read more than 4k into encinput. So if the ping payload is greater
than 4k this will just loop forever.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 6/6] io: Reply to ping frames

[Brandon Carpenter]

On Tue, Sep 12, 2017 at 2:01 AM, Daniel P. Berrange 
<berrange@redhat.com> wrote:
> The problem is in the qio_channel_websock_read_wire method we refuse 
> to read more than 4k into encinput. So if the ping payload is greater 
> than 4k this will just loop forever.

The RFC limits the payload length of control messages to 126 bytes, , 
limiting the total message size to 132 bytes. This is enforced in 
qio_channel_websock_decode_header(). If anything larger is sent in a 
control message, including pings, the connection is immediately closed.
--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060


-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH] io: Always remove an old channel watch before adding a new one.

[Paolo Bonzini]

On 24/07/2017 20:15, Brandon Carpenter wrote:
> Also set saved handle to zero when removing without adding a new watch.
> 
> Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> ---
>  ui/vnc-auth-vencrypt.c | 3 +++
>  ui/vnc-ws.c            | 6 ++++++
>  ui/vnc.c               | 4 ++++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
> index ffaab57550..c3eece4fa7 100644
> --- a/ui/vnc-auth-vencrypt.c
> +++ b/ui/vnc-auth-vencrypt.c
> @@ -77,6 +77,9 @@ static void vnc_tls_handshake_done(QIOTask *task,
>          vnc_client_error(vs);
>          error_free(err);
>      } else {
> +        if (vs->ioc_tag) {
> +            g_source_remove(vs->ioc_tag);
> +        }
>          vs->ioc_tag = qio_channel_add_watch(
>              vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
>          start_auth_vencrypt_subauth(vs);
> diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
> index f530cd5474..eaf309553c 100644
> --- a/ui/vnc-ws.c
> +++ b/ui/vnc-ws.c
> @@ -36,6 +36,9 @@ static void vncws_tls_handshake_done(QIOTask *task,
>          error_free(err);
>      } else {
>          VNC_DEBUG("TLS handshake complete, starting websocket handshake\n");
> +        if (vs->ioc_tag) {
> +            g_source_remove(vs->ioc_tag);
> +        }
>          vs->ioc_tag = qio_channel_add_watch(
>              QIO_CHANNEL(vs->ioc), G_IO_IN, vncws_handshake_io, vs, NULL);
>      }
> @@ -97,6 +100,9 @@ static void vncws_handshake_done(QIOTask *task,
>      } else {
>          VNC_DEBUG("Websock handshake complete, starting VNC protocol\n");
>          vnc_start_protocol(vs);
> +        if (vs->ioc_tag) {
> +            g_source_remove(vs->ioc_tag);
> +        }
>          vs->ioc_tag = qio_channel_add_watch(
>              vs->ioc, G_IO_IN, vnc_client_io, vs, NULL);
>      }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index eb91559b6b..ec86d6ad97 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1121,6 +1121,7 @@ static void vnc_disconnect_start(VncState *vs)
>      vnc_set_share_mode(vs, VNC_SHARE_MODE_DISCONNECTED);
>      if (vs->ioc_tag) {
>          g_source_remove(vs->ioc_tag);
> +        vs->ioc_tag = 0;
>      }
>      qio_channel_close(vs->ioc, NULL);
>      vs->disconnecting = TRUE;
> @@ -2931,6 +2932,9 @@ static void vnc_connect(VncDisplay *vd, QIOChannelSocket *sioc,
>      VNC_DEBUG("New client on socket %p\n", vs->sioc);
>      update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
>      qio_channel_set_blocking(vs->ioc, false, NULL);
> +    if (vs->ioc_tag) {
> +        g_source_remove(vs->ioc_tag);
> +    }
>      if (websocket) {
>          vs->websocket = 1;
>          if (vd->tlscreds) {
> 


Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] io: Always remove an old channel watch before adding a new one.

[Daniel P. Berrange]

On Mon, Jul 24, 2017 at 11:15:44AM -0700, Brandon Carpenter wrote:
> Also set saved handle to zero when removing without adding a new watch.
> 
> Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> ---
>  ui/vnc-auth-vencrypt.c | 3 +++
>  ui/vnc-ws.c            | 6 ++++++
>  ui/vnc.c               | 4 ++++
>  3 files changed, 13 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] io: Always remove an old channel watch before adding a new one.

[Brandon Carpenter]

I haven't seen this patch hit master yet and am about to submit a patch 
set that is dependent on this one because it triggers the bug fixed by 
this patch, causing a segmentation fault. Is it preferred that I 
include this patch in that series with the Reviewed-by: tags or to just 
reference this patch in the cover letter?

Thanks.
--
Brandon Carpenter | Software Engineer
Cypherpath, Inc.
400 Columbia Point Drive Ste 101 | Richland, Washington USA
Office: (650) 713-3060

On Tue, Jul 25, 2017 at 1:36 AM, Daniel P. Berrange 
<berrange@redhat.com> wrote:
> On Mon, Jul 24, 2017 at 11:15:44AM -0700, Brandon Carpenter wrote:
>>  Also set saved handle to zero when removing without adding a new 
>> watch.
>> 
>>  Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
>>  ---
>>   ui/vnc-auth-vencrypt.c | 3 +++
>>   ui/vnc-ws.c            | 6 ++++++
>>   ui/vnc.c               | 4 ++++
>>   3 files changed, 13 insertions(+)
> 
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> 
> 
> 
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    
> https://www.instagram.com/dberrange :|

-- 


CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and may contain proprietary, 
confidential or privileged information or otherwise be protected by law. 
Any unauthorized review, use, disclosure or distribution is prohibited. If 
you are not the intended recipient, please notify the sender and destroy 
all copies and the original message.

Re: [Qemu-devel] [PATCH] io: Always remove an old channel watch before adding a new one.

[Daniel P. Berrange]

On Fri, Sep 08, 2017 at 09:18:04AM -0700, Brandon Carpenter wrote:
> I haven't seen this patch hit master yet and am about to submit a patch set
> that is dependent on this one because it triggers the bug fixed by this
> patch, causing a segmentation fault. Is it preferred that I include this
> patch in that series with the Reviewed-by: tags or to just reference this
> patch in the cover letter?

Yes, if you re-post the patch as part of a larger just, add any
Reviewed-by tags to the commit message when you re-post.

> On Tue, Jul 25, 2017 at 1:36 AM, Daniel P. Berrange <berrange@redhat.com>
> wrote:
> > On Mon, Jul 24, 2017 at 11:15:44AM -0700, Brandon Carpenter wrote:
> > >  Also set saved handle to zero when removing without adding a new
> > > watch.
> > > 
> > >  Signed-off-by: Brandon Carpenter <brandon.carpenter@cypherpath.com>
> > >  ---
> > >   ui/vnc-auth-vencrypt.c | 3 +++
> > >   ui/vnc-ws.c            | 6 ++++++
> > >   ui/vnc.c               | 4 ++++
> > >   3 files changed, 13 insertions(+)
> > 
> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> > 
> > 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|