[Spice-devel] [PATCH spice 3/3] sndworker: check for over-sized messages and simplify a bit the code

[Marc-André Lureau]

If the client sends messages that don't fit in the receive_data.buf,
we don't want to assert, but instead disconnect the misbehaving client.

I could reproduce this crash by changing the frame size in the spice-gtk
client, but there might be other conditions where it happened, we
shouldn't crash in this case.
---
 server/snd_worker.c |   34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/server/snd_worker.c b/server/snd_worker.c
index a0bbd3a..591a023 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -195,8 +195,6 @@ typedef struct RecordChannel {
 static SndWorker *workers;
 static uint32_t playback_compression = SPICE_AUDIO_DATA_MODE_CELT_0_5_1;
 
-static void snd_receive(void* data);
-
 static SndChannel *snd_channel_get(SndChannel *channel)
 {
     channel->refs++;
@@ -417,14 +415,11 @@ static int snd_record_handle_message(SndChannel *channel, size_t size, uint32_t
     return TRUE;
 }
 
-static void snd_receive(void* data)
+static void snd_receive(SndChannel *channel)
 {
-    SndChannel *channel = (SndChannel*)data;
     SpiceDataHeaderOpaque *header;
 
-    if (!channel) {
-        return;
-    }
+    spice_return_if_fail(channel != NULL); // FIXME: assert
 
     header = &channel->channel_client->incoming.header;
 
@@ -464,11 +459,10 @@ static void snd_receive(void* data)
                 header->data = msg_start;
                 n = channel->receive_data.now - msg_start;
 
-                if (n < header->header_size ||
-                    n < header->header_size + header->get_msg_size(header)) {
+                if (n < header->header_size + header->get_msg_size(header)) {
                     break;
                 }
-                parsed = channel->parser((void *)data, data + header->get_msg_size(header),
+                parsed = channel->parser(data, data + header->get_msg_size(header),
                                          header->get_msg_type(header),
                                          SPICE_VERSION_MINOR, &parsed_size, &parsed_free);
                 if (parsed == NULL) {
@@ -486,14 +480,18 @@ static void snd_receive(void* data)
                 channel->receive_data.message_start = msg_start + header->header_size +
                                                      header->get_msg_size(header);
             }
-            if (channel->receive_data.now == channel->receive_data.message_start) {
-                channel->receive_data.now = channel->receive_data.buf;
-                channel->receive_data.message_start = channel->receive_data.buf;
-            } else if (channel->receive_data.now == channel->receive_data.end) {
-                memcpy(channel->receive_data.buf, channel->receive_data.message_start, n);
-                channel->receive_data.now = channel->receive_data.buf + n;
-                channel->receive_data.message_start = channel->receive_data.buf;
+
+            /* n could eventually be sizeof(channel->receive_data.buf) if the message doesn't fit
+               Do not abort in this case, it's just an 'incompatible/invalid' client */
+            if (n >= sizeof(channel->receive_data.buf)) {
+                spice_printerr("failed to receive message, it is too large for this channel");
+                snd_disconnect_channel(channel);
+                return;
             }
+
+            memmove(channel->receive_data.buf, channel->receive_data.message_start, n);
+            channel->receive_data.now = channel->receive_data.buf + n;
+            channel->receive_data.message_start = channel->receive_data.buf;
         }
     }
 }
@@ -1330,7 +1328,7 @@ SPICE_GNUC_VISIBLE uint32_t spice_server_record_get_samples(SpiceRecordInstance
 
     if (len < bufsize) {
         SndWorker *worker = record_channel->base.worker;
-        snd_receive(record_channel);
+        snd_receive(channel);
         if (!worker->connection) {
             return 0;
         }
-- 
1.7.10