[Spice-devel] [PATCH spice 1/3] spice-server: Add the ability to filter agent messages

[Alon Levy]

On Fri, Mar 25, 2011 at 03:07:07PM +0100, Hans de Goede wrote:
> Hi,
> 
> Thanks for the review!
> 

And thanks for the clarification, corrected on both cases, so:

ACK series.

> On 03/25/2011 02:52 PM, Alon Levy wrote:
> >On Thu, Mar 24, 2011 at 05:39:46PM +0100, Hans de Goede wrote:
> >>---
> >>  server/Makefile.am        |    2 +
> >>  server/agent-msg-filter.c |   85 +++++++++++++++++++++++++++++++++++++++++++++
> >>  server/agent-msg-filter.h |   45 ++++++++++++++++++++++++
> >>  server/reds.c             |   32 ++++++++++++++++-
> >>  4 files changed, 162 insertions(+), 2 deletions(-)
> >>  create mode 100644 server/agent-msg-filter.c
> >>  create mode 100644 server/agent-msg-filter.h
> >>
> >>diff --git a/server/Makefile.am b/server/Makefile.am
> >>index 7ab8c3d..37ff183 100644
> >>--- a/server/Makefile.am
> >>+++ b/server/Makefile.am
> >>@@ -95,6 +95,8 @@ SMARTCARD_SRCS =
> >>  endif
> >>
> >>  libspice_server_la_SOURCES =			\
> >>+	agent-msg-filter.c			\
> >>+	agent-msg-filter.h			\
> >>  	demarshallers.h				\
> >>  	glz_encoder.c				\
> >>  	glz_encoder_config.h			\
> >>diff --git a/server/agent-msg-filter.c b/server/agent-msg-filter.c
> >>new file mode 100644
> >>index 0000000..3867d11
> >>--- /dev/null
> >>+++ b/server/agent-msg-filter.c
> >>@@ -0,0 +1,85 @@
> >>+/*
> >>+   Copyright (C) 2011 Red Hat, Inc.
> >>+
> >>+   This library is free software; you can redistribute it and/or
> >>+   modify it under the terms of the GNU Lesser General Public
> >>+   License as published by the Free Software Foundation; either
> >>+   version 2.1 of the License, or (at your option) any later version.
> >>+
> >>+   This library is distributed in the hope that it will be useful,
> >>+   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>+   Lesser General Public License for more details.
> >>+
> >>+   You should have received a copy of the GNU Lesser General Public
> >>+   License along with this library; if not, see<http://www.gnu.org/licenses/>.
> >>+
> >>+   Red Hat Authors:
> >>+        hdegoede at redhat.com
> >>+*/
> >>+
> >>+#include<string.h>
> >>+#include "red_common.h"
> >>+#include "agent-msg-filter.h"
> >>+
> >>+void agent_msg_filter_init(struct AgentMsgFilter *filter, int copy_paste)
> >>+{
> >>+    memset(filter, 0, sizeof(*filter));
> >
> >You are explicitly relying on AGENT_MSG_FILTER_OK == 0, would be nice
> >to set filter->Result = AGENT_MSG_FILTER_OK explicitly.
> >
> 
> Er, no result will not be used until we've gone through the switch case in
> agent_msg_filter_process_data at least once, at which point it has always
> been explicitly set to one of the values from the enum.
> 
> >>+    filter->copy_paste_enabled = copy_paste;
> >>+}
> >>+
> >>+int agent_msg_filter_process_data(struct AgentMsgFilter *filter,
> >>+                                  uint8_t *data, uint32_t len)
> >>+{
> >>+    struct VDAgentMessage msg_header;
> >>+
> >>+    if (len>  VD_AGENT_MAX_DATA_SIZE) {
> >>+        red_printf("invalid agent message: too large");
> >>+        return AGENT_MSG_FILTER_PROTO_ERROR;
> >>+    }
> >>+
> >>+    /* Are we expecting more data from a previous message? */
> >>+    if (filter->msg_data_to_read) {
> >>+data_to_read:
> >>+        if (len>  filter->msg_data_to_read) {
> >>+            red_printf("invalid agent message: data exceeds size from header");
> >>+            return AGENT_MSG_FILTER_PROTO_ERROR;
> >>+        }
> >>+        filter->msg_data_to_read -= len;
> >>+        return filter->result;
> >>+    }
> >>+
> >>+    if (len<  sizeof(msg_header)) {
> >
> >Is it impossible to get a partial write that is smaller then the size
> >of the header? This also applies for a read with multiple messages
> >ending with a partial header.
> >
> 
> Note that at the point agent_msg_filter_process_data gets called, the
> (de)multiplexing from/into chunks has already been done. So we are being
> fed data one chunk at a time. So what these checks check is that:
> -The first chunk of a (possibly multi chunk spanning) message contains a
>  complete header
> -The last chunk of a message does not push to total amount of message data
>  over the size specified in the messages header
> 
> The linux vdagent has the same sanity checks. If either one of these 2
> fails, things are seriously wrong.
> 
> Regards,
> 
> Hans
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel