[Spice-devel] [PATCH spice-server] agent: fix mishandling of agent data received from the client after agent disconnection

[Hans de Goede]

Hi,

Thanks for your work on this!

Fix looks good, ACK.

Regards,

Hans




On 11/30/2012 05:28 PM, Yonit Halperin wrote:
> The server can receive from the client agent data even when the agent
> is disconnected. This can happen if the client sends the agent data
> before it receives the AGENT_DISCONNECTED msg. We should receive and handle such msgs, instead
> of disconnecting the client.
> This bug can also lead to a server crash if the agent gets reconnected
> fast enough, and it receives an agent data msg from the client before MSGC_AGENT_START.
>
> upstream bz#55726
> rhbz#881980
> ---
>   server/reds.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index b99d01f..3a8a28d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -474,7 +474,11 @@ static void reds_reset_vdp(void)
>       /* Reset read filter to start with clean state when the agent reconnects */
>       agent_msg_filter_init(&state->read_filter, agent_copypaste, TRUE);
>       /* Throw away pending chunks from the current (if any) and future
> -       messages written by the client */
> +     * messages written by the client.
> +     * TODO: client should clear its agent messages queue when the agent
> +     * is disconnect. Currently, when and agent gets disconnected and reconnected,
> +     * messeges that were directed to the previous instance of the agent continues
> +     * to be sent from the client. This TODO will require server, protocol, and client changes */
>       state->write_filter.result = AGENT_MSG_FILTER_DISCARD;
>       state->write_filter.discard_all = TRUE;
>       state->client_agent_started = FALSE;
> @@ -996,8 +1000,15 @@ uint8_t *reds_get_agent_data_buffer(MainChannelClient *mcc, size_t size)
>       VDIPortState *dev_state = &reds->agent_state;
>       RedClient *client;
>
> -    if (!dev_state->base) {
> -        return NULL;
> +    if (!dev_state->client_agent_started) {
> +        /*
> +         * agent got disconnected, and possibly got reconnected, but we still can receive
> +         * msgs that are addressed to the agent's old instance, in case they were
> +         * sent by the client before it received the AGENT_DISCONNECTED msg.
> +         * In such case, we will receive and discard the msgs (reds_reset_vdp takes care
> +         * of setting state->write_filter.result = AGENT_MSG_FILTER_DISCARD).
> +         */
> +        return spice_malloc(size);
>       }
>
>       spice_assert(dev_state->recv_from_client_buf == NULL);
> @@ -1013,8 +1024,12 @@ void reds_release_agent_data_buffer(uint8_t *buf)
>   {
>       VDIPortState *dev_state = &reds->agent_state;
>
> -    spice_assert(buf == dev_state->recv_from_client_buf->buf + sizeof(VDIChunkHeader));
> +    if (!dev_state->recv_from_client_buf) {
> +        free(buf);
> +        return;
> +    }
>
> +    spice_assert(buf == dev_state->recv_from_client_buf->buf + sizeof(VDIChunkHeader));
>       if (!dev_state->recv_from_client_buf_pushed) {
>           spice_char_device_write_buffer_release(reds->agent_state.base,
>                                                  dev_state->recv_from_client_buf);
> @@ -1064,8 +1079,6 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
>       VDIChunkHeader *header;
>       int res;
>
> -    spice_assert(message == reds->agent_state.recv_from_client_buf->buf + sizeof(VDIChunkHeader));
> -
>       res = agent_msg_filter_process_data(&reds->agent_state.write_filter,
>                                           message, size);
>       switch (res) {
> @@ -1080,6 +1093,9 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
>           reds_disconnect();
>           return;
>       }
> +
> +    spice_assert(reds->agent_state.recv_from_client_buf);
> +    spice_assert(message == reds->agent_state.recv_from_client_buf->buf + sizeof(VDIChunkHeader));
>       // TODO - start tracking agent data per channel
>       header =  (VDIChunkHeader *)dev_state->recv_from_client_buf->buf;
>       header->port = VDP_CLIENT_PORT;
>