[Spice-devel] [spice-gtk v1] channel-usbredir: avoid calling memcpy() will NULL src

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> Code built with address sanitizer has runtime error:
>  > channel-usbredir.c:642:5: runtime error: null pointer passed
>  > as argument 2, which is declared to never be null
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/channel-usbredir.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 1f791bc..7c48ecb 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -635,9 +635,9 @@ static int usbredir_read_callback(void *user_data,
> uint8_t *data, int count)
>      SpiceUsbredirChannel *channel = user_data;
>      SpiceUsbredirChannelPrivate *priv = channel->priv;
>  
> -    if (priv->read_buf_size < count) {
> -        count = priv->read_buf_size;
> -    }
> +    count = MIN(priv->read_buf_size, count);

Technically this part is just a style change but
is clearly doing a minimum operation.

> +    if (count == 0)
> +        return 0;
>  
>      memcpy(data, priv->read_buf, count);
>  

memcpy should not dereference any 0-byte area but I agree is better to
silence the sanitizer and other tools.

Looking at the code there can be a side effects.
If the usbredir send a 0-byte package you get read_buf_size == 0 and
read_buf != NULL, processing this message lead to have read_buf != NULL
now which can trigger a failure in usbredir_handle_msg (see code after
the memcpy). Don't know if this is possible. Maybe is safer to do a

  if (count) {
      memcpy(data, priv->read_buf, count);
  }

Frediano