[Spice-commits] 3 commits - spice-protocol vdagent/vdagent.cpp

[GitLab Mirror]

 spice-protocol      |    2 -
 vdagent/vdagent.cpp |   55 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 37 insertions(+), 20 deletions(-)

New commits:
commit 18cbe9f306f6dcfeeb4952e278dd88e2520d87f6
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Fri Jun 29 08:02:02 2018 +0100

    Replace an assert with proper handling code
    
    Make sure the condition is handled properly.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index 9fbff3d..7b3720d 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -1412,7 +1412,11 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
 
         // got just the start, start to collapse all chunks into a
         // single buffer
-        ASSERT(chunk->hdr.size < msg_size);
+        if (chunk->hdr.size >= msg_size) {
+            vd_printf("Invalid VDAgentMessage message");
+            _running = false;
+            return;
+        }
         _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
         memcpy(_in_msg, chunk->data, chunk->hdr.size);
         _in_msg_pos = chunk->hdr.size;
commit 3e8cab6da1a1572db9a91ee21687ab5dca7671b1
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 26 07:53:51 2018 +0100

    Minor overflow checks improvements
    
    Although source of these data should be safe, improve data checks
    to avoid some overflows and make the code more robust.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index cf492cc..9fbff3d 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -1368,7 +1368,7 @@ VOID VDAgent::read_completion(DWORD err, DWORD bytes, LPOVERLAPPED overlapped)
         count = sizeof(VDIChunk) - a->_read_pos;
     } else if (a->_read_pos == sizeof(VDIChunk)) {
         count = chunk->hdr.size;
-        if (a->_read_pos + count > sizeof(a->_read_buf)) {
+        if (count > sizeof(a->_read_buf) - a->_read_pos) {
             vd_printf("chunk is too large, size %u port %u", chunk->hdr.size, chunk->hdr.port);
             a->_running = false;
             return;
@@ -1420,6 +1420,12 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
     }
 
     // the previous chunk was a partial message, so append this chunk to the previous chunk
+    if (chunk->hdr.size > sizeof(VDAgentMessage) + _in_msg->size - _in_msg_pos) {
+        vd_printf("Invalid VDAgentMessage message");
+        _running = false;
+        return;
+    }
+
     memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk->hdr.size);
     _in_msg_pos += chunk->hdr.size;
     // update clipboard tick on each clipboard chunk for timeout setting
commit 873464cecceb1895a620ca3606004f7c856cfd79
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Fri Jun 29 07:50:44 2018 +0100

    Reduce indentation returning earlier
    
    Also add some comments.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

diff --git a/spice-protocol b/spice-protocol
index 666b5c5..dca5931 160000
--- a/spice-protocol
+++ b/spice-protocol
@@ -1 +1 @@
-Subproject commit 666b5c5780acf3176a9cff61ad549d30bb1b9824
+Subproject commit dca5931a3ac405139b8d05da68544cc43ae56d83
diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index 78c42d1..cf492cc 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -1393,6 +1393,7 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
 {
     //FIXME: currently assumes that multi-part msg arrives only from client port
     if (_in_msg_pos == 0 || chunk->hdr.port == VDP_SERVER_PORT) {
+        // ignore the chunk if too short
         if (chunk->hdr.size < sizeof(VDAgentMessage)) {
             return;
         }
@@ -1404,28 +1405,34 @@ void VDAgent::handle_chunk(VDIChunk* chunk)
         }
         uint32_t msg_size = sizeof(VDAgentMessage) + msg->size;
         if (chunk->hdr.size == msg_size) {
+            // we got an entire message, handle it
             dispatch_message(msg, chunk->hdr.port);
-        } else {
-            ASSERT(chunk->hdr.size < msg_size);
-            _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
-            memcpy(_in_msg, chunk->data, chunk->hdr.size);
-            _in_msg_pos = chunk->hdr.size;
-        }
-    } else {
-        memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk->hdr.size);
-        _in_msg_pos += chunk->hdr.size;
-        // update clipboard tick on each clipboard chunk for timeout setting
-        if (_in_msg->type == VD_AGENT_CLIPBOARD && _clipboard_tick) {
-            _clipboard_tick = GetTickCount();
+            return;
         }
-        if (_in_msg_pos == sizeof(VDAgentMessage) + _in_msg->size) {
-            if (_in_msg->type == VD_AGENT_CLIPBOARD && !_clipboard_tick) {
-                vd_printf("Clipboard received but dropped due to timeout");
-            } else {
-                dispatch_message(_in_msg, 0);
-            }
-            cleanup_in_msg();
+
+        // got just the start, start to collapse all chunks into a
+        // single buffer
+        ASSERT(chunk->hdr.size < msg_size);
+        _in_msg = (VDAgentMessage*)new uint8_t[msg_size];
+        memcpy(_in_msg, chunk->data, chunk->hdr.size);
+        _in_msg_pos = chunk->hdr.size;
+        return;
+    }
+
+    // the previous chunk was a partial message, so append this chunk to the previous chunk
+    memcpy((uint8_t*)_in_msg + _in_msg_pos, chunk->data, chunk->hdr.size);
+    _in_msg_pos += chunk->hdr.size;
+    // update clipboard tick on each clipboard chunk for timeout setting
+    if (_in_msg->type == VD_AGENT_CLIPBOARD && _clipboard_tick) {
+        _clipboard_tick = GetTickCount();
+    }
+    if (_in_msg_pos == sizeof(VDAgentMessage) + _in_msg->size) {
+        if (_in_msg->type == VD_AGENT_CLIPBOARD && !_clipboard_tick) {
+            vd_printf("Clipboard received but dropped due to timeout");
+        } else {
+            dispatch_message(_in_msg, 0);
         }
+        cleanup_in_msg();
     }
 }