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

[Hans de Goede]

Hi,

Thanks for the review!

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