[Spice-devel] [PATCH spice-server v2 4/7] smartcard: Fix parsing multiple messages from the device

[Victor Toso]

Hi,

Code seems fine, I'd change a bit the commit log just to be
straight forward with what this is fixing/improving. I'm
suggesting to split what might be a bugfix but feel free to
correct me if I'm mistaken ;)

On Tue, Oct 08, 2019 at 06:39:21PM +0100, Frediano Ziglio wrote:
> If the server is busy or the guest write multiple requests with
> a single operation it could happen that we receive multiple
> requests with a single read.

This patch handles the scenario when a single read to guest
device brings multiple requests to be handled. When this happens,
we will iterate till all requests are handled and no more
requests can be read from guest device.

> This will make "remaining" > 0.
> Use memmove instead of memcpy as the buffer can overlap if the
> second request if bigger than the first.
> "buf_pos" points to the point of the buffer after we read, if
> we want the first part of the next request is "buf_pos - remaining".
> Same consideration setting "buf_pos" for the next iteration.
> Also check the loop condition. If the remaining buffer contains
> a full request we don't need to read other bytes (note that there
> could be no bytes left), just parse the request.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/smartcard.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/server/smartcard.c b/server/smartcard.c
> index 4c5bba07d..340118e18 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -130,19 +130,28 @@ static RedPipeItem *smartcard_read_msg_from_device(RedCharDevice *self,
>      RedCharDeviceSmartcard *dev = RED_CHAR_DEVICE_SMARTCARD(self);
>      SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
>      VSCMsgHeader *vheader = (VSCMsgHeader*)dev->priv->buf;
> -    int n;
>      int remaining;
>      int actual_length;
>  
> -    while ((n = sif->read(sin, dev->priv->buf_pos, dev->priv->buf_size - dev->priv->buf_used)) > 0) {
> +    while (true) {
>          RedMsgItem *msg_to_client;
>  
> -        dev->priv->buf_pos += n;
> -        dev->priv->buf_used += n;
> -        if (dev->priv->buf_used < sizeof(VSCMsgHeader)) {
> -            continue;
> +        // it's possible we already got a full message from a previous partial
> +        // read. In this case we don't need to read any byte
> +        if (dev->priv->buf_used < sizeof(VSCMsgHeader) ||
> +            dev->priv->buf_used - sizeof(VSCMsgHeader) < ntohl(vheader->length)) {
> +            int n = sif->read(sin, dev->priv->buf_pos, dev->priv->buf_size - dev->priv->buf_used);
> +            if (n <= 0) {
> +                break;
> +            }
> +            dev->priv->buf_pos += n;
> +            dev->priv->buf_used += n;
> +
> +            if (dev->priv->buf_used < sizeof(VSCMsgHeader)) {
> +                continue;
> +            }
> +            smartcard_read_buf_prepare(dev, vheader);
>          }
> -        smartcard_read_buf_prepare(dev, vheader);
>          actual_length = ntohl(vheader->length);
>          if (dev->priv->buf_used - sizeof(VSCMsgHeader) < actual_length) {
>              continue;
> @@ -150,9 +159,9 @@ static RedPipeItem *smartcard_read_msg_from_device(RedCharDevice *self,
>          msg_to_client = smartcard_char_device_on_message_from_device(dev, vheader);
>          remaining = dev->priv->buf_used - sizeof(VSCMsgHeader) - actual_length;
>          if (remaining > 0) {
> -            memcpy(dev->priv->buf, dev->priv->buf_pos, remaining);
> +            memmove(dev->priv->buf, dev->priv->buf_pos - remaining, remaining);

This should be addressed in a separated patch as bug, If I
understand correctly. Even before remaining could be > 0 but what
was being memcpy was trash instead bytes being read by
sif->read()

>          }
> -        dev->priv->buf_pos = dev->priv->buf;
> +        dev->priv->buf_pos = dev->priv->buf + remaining;

This as well, again, If I'm not missing anything.

Cheers,


>          dev->priv->buf_used = remaining;
>          if (msg_to_client) {
>              return &msg_to_client->base;
> -- 
> 2.21.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20191009/5d3ac000/attachment.sig>